fix: self-DM/sibling decryption deadlock (retry shape + session recovery + peer pkmsg identity) - #635
Conversation
`prepare_dm_retry_stanza` was emitting the fanout-shaped envelope (`<message><participants><to jid=...><enc>...`) for single-device retries. WAWebSendMsgCreateDeviceStanza.createUserDeviceMsgStanza puts the `<enc>` directly under `<message>` with the `recipient` attribute set, and the server rejects the fanout shape on retries with `ack error="479"` (SmaxInvalid). After the PR #634 self-DM fix the original send finally reached the user's primary phone, which then issued retry receipts — every retry resend the bot sent back got 479, so the Android app never received the message even though the same account on WhatsApp Web (device 94) decrypted the original fine. Verified live on prod container `k8awqjsgww2lnkt89urp3de1` (`docker logs --since 4h | grep -c SmaxInvalid` = 12), with every 479 firing on a retry resend (see log `k8awqjsgww2lnkt89urp3de1-220743194622-...txt` line 4385 vs 4434). Changes - `wacore::send::prepare_dm_retry_stanza`: * Drop the `<participants><to jid=...>` wrapper; `<enc>` becomes a direct child of `<message>`. * Replace the unused `requester_jid` parameter with `recipient_jid` and emit `recipient="..."` on the message (WA Web's `recipient: USER_JID(g)`). * Apply `decrypt-fail="hide"` on `<enc>` via the same `should_hide_decrypt_fail_for_send` predicate used by the fanout path, matching `WAWebE2EProtoUtils.decryptFailAttributeFromProtobuf`. - `src/retry.rs`: pass `info.chat.clone()` as the recipient. Tests - `dm_retry_emits_enc_directly_under_message_with_recipient` (new) — pins the WA-Web shape; failed on `main` before the refactor, passes after. - Updated `dm_retry_pkmsg_targets_single_device`, `dm_retry_pkmsg_with_account_has_device_identity`, and `dm_retry_preserves_edit_attribute` to reflect the new shape. Out of scope - `prepare_group_retry_stanza` (already uses direct-child enc; missing `decrypt-fail`/`<meta>` but no prod evidence of a server reject). - DSM wrapping of the payload for self-DM retry (payload-level, does not cause 479). - Reporting-token element on non-self DM retries (payload-level).
|
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
WalkthroughForward receipt recipient into DM resend; add pkmsg preflight/account gating and stanza shape changes; add per‑JID throttled session-recreate fallback; attempt PN→LID migration on BadMac/InvalidMessage; make libsignal decryption transactional; expand tests and benches. ChangesDM Retry Stanza & Peer Stanza
Session Recreation Throttling & Retry Handling
LID↔PN Session Migration
Transactional Decrypt & Message-Level Migration
PDO peer-target selection & send wiring
Time ops & examples
Estimated code review effort Possibly related PRs
🚥 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 |
![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 `@src/retry.rs`:
- Around line 472-481: The recipient JID used for DM retry stanzas is taken from
info.chat which is wrong when take_recent_message() returned an alternate cached
namespace (alt_chat); change the code so you prefer the cached message namespace
(the alt_chat or the namespace stored on the recent message) as recipient_jid
when present, and only fall back to info.chat otherwise, then pass that chosen
recipient_jid into wacore::send::prepare_dm_retry_stanza; locate the logic
around take_recent_message(), the alt_chat variable (or the recent message
object), and the call site prepare_dm_retry_stanza to implement this conditional
selection.
🪄 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: 18508ad8-9c05-4aec-b646-d57aa7afbe22
📒 Files selected for processing (2)
src/retry.rswacore/src/send.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: cda07efdc4
ℹ️ 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".
| /// Single-device retry resend for a DM, mirroring | ||
| /// `WAWebSendMsgCreateDeviceStanza.createUserDeviceMsgStanza`. The `<enc>` | ||
| /// is a direct child of `<message>` (no `<participants>` fanout wrapper), | ||
| /// `recipient` carries the original message's destination, and | ||
| /// `decrypt-fail="hide"` is set for the same message kinds that hide it on |
There was a problem hiding this comment.
This newly added block violates the repository’s documented comment convention in AGENTS.md (“dont be so verbose, also only explain why, not what”), which is treated as a required review guideline in this project. Keeping this level of narrative detail in code comments makes maintenance harder and sets a conflicting precedent for future changes; please trim it to a short rationale-only note.
Useful? React with 👍 / 👎.
…ents
- `wacore::send`: remove a second `#[allow(clippy::too_many_arguments)]`
that landed during the docstring rewrite (clippy CI -D warnings).
- Trim verbose docstrings on `prepare_dm_retry_stanza` and the new
regression test.
- `tests/e2e/tests/retry_dm_multidevice.rs`:
* Adjust the `<participants>` helpers to the new direct-`<enc>` shape.
* Assert `participant_target_count == 0` and `<enc>` is a direct
child of `<message>` — guards against any future regression to the
fanout shape.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/tests/retry_dm_multidevice.rs (1)
101-126: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the retry
recipientattribute too.This only proves the device-targeted
topath. The new cross-layer contract in this PR is therecipienthandoff, and this test will not catch a regression there.📌 Suggested assertion
let retry_to = retry_node .attrs() .optional_jid("to") .expect("Retry resend should have a 'to' attribute"); + let retry_recipient = retry_node + .attrs() + .optional_jid("recipient") + .expect("Retry resend should have a 'recipient' attribute"); assert!( retry_to.is_same_user_as(&jid_b), "Retry resend 'to' should target the same user (got {retry_to}, expected user {})", jid_b ); + assert!( + retry_recipient.is_same_user_as(&jid_b), + "Retry resend 'recipient' should preserve the original chat JID (got {retry_recipient}, expected user {})", + jid_b + );🤖 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 `@tests/e2e/tests/retry_dm_multidevice.rs` around lines 101 - 126, The test currently only asserts the stanza 'to' JID; add an assertion that the retry stanza also carries a 'recipient' attribute and that it targets the same user as jid_b. Locate the retry_node usage in this test and call attrs().optional_jid("recipient") (similar to the existing optional_jid("to")) to ensure it is present, then assert recipient.is_same_user_as(&jid_b) with an explanatory message referencing retry_node and jid_b so regressions in the cross-layer 'recipient' handoff are caught.
🤖 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.
Outside diff comments:
In `@tests/e2e/tests/retry_dm_multidevice.rs`:
- Around line 101-126: The test currently only asserts the stanza 'to' JID; add
an assertion that the retry stanza also carries a 'recipient' attribute and that
it targets the same user as jid_b. Locate the retry_node usage in this test and
call attrs().optional_jid("recipient") (similar to the existing
optional_jid("to")) to ensure it is present, then assert
recipient.is_same_user_as(&jid_b) with an explanatory message referencing
retry_node and jid_b so regressions in the cross-layer 'recipient' handoff are
caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f6a71dc-60a8-4e88-ae3c-f89405f68b5d
📒 Files selected for processing (2)
tests/e2e/tests/retry_dm_multidevice.rswacore/src/send.rs
new DM retry shape The fix for the prod 479 collapsed the DM retry stanza onto a single `<enc>` directly under `<message>` (the WAWebSendMsgCreateDeviceStanza shape). bartender's mock DM router still expects the `<participants>` fanout wrapper to deliver the resend through to the destination client, so this end-to-end test never sees the recovered text and times out. The shape itself is pinned at the unit level by `dm_retry_emits_enc_directly_under_message_with_recipient` in `wacore::send::tests`. Drop the `#[ignore]` once the mock server fans out the bare-`<enc>` retry shape.
CodeRabbit caught that `recipient_jid = info.chat` is wrong on the PN↔LID alt-namespace path — info.chat reflects the receipt namespace, not the namespace the original outbound message was sent under, so the retry stanza's `recipient` could disagree with the original `to`. Cross-checking WAWebHandleRetryRequest shows a tighter contract: `f && (k.recipient = f)` — WA Web only sets the attribute when the incoming retry receipt carried one, and uses its value verbatim. For non-self DM the receipt has no `recipient` and the resend drops it. Changes - `RetryChatInfo`: carry the receipt's `recipient` attribute as `Option<Jid>` (`recipient` field) alongside `original_from` and `chat`. - `prepare_dm_retry_stanza`: accept `recipient_jid: Option<Jid>` and only emit the attribute when `Some`. Caller passes the parsed receipt attr. - `src/retry.rs`: drop the `info.chat` derivation; pass `info.recipient.clone()` straight through.
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/send.rs`:
- Around line 2911-2951: Update the test
dm_retry_emits_enc_directly_under_message_with_recipient to use different JIDs
for the "to" (requesting device) and the "recipient" (original chat) so the test
fails if the code incorrectly wires recipient from to_jid; when calling
prepare_dm_retry_stanza supply distinct Jid values for to and
requester/recipient (e.g., make `to` a device JID and `requester`/`recipient` a
different chat JID), then add an assertion that they are not equal and assert
the stanza's recipient attribute equals the original chat JID (use
n.attrs().optional_string("recipient") and compare to the expected recipient),
referencing the test name and prepare_dm_retry_stanza to locate where to change
inputs and assertions.
🪄 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: a6347cdf-84f8-4c42-83c4-e5e5865bba2d
📒 Files selected for processing (2)
src/retry.rswacore/src/send.rs
…ow parity) Production hit a stuck state with a 480KB session blob and 6 archived states for a sibling device, where Android and the bot's ratchet chains diverged silently. WA Web's `updateLocalSignalSession` only deletes on regId mismatch or base-key collision (per-message-id check), neither of which fires when both sides keep the same regId and the peer gives up before retry #3. Whatsmeow has the missing escape hatch in `shouldRecreateSession` (`retry.go:158`): when the incoming retry receipt has no `<keys>` and `retry_count >= 2`, force a fresh prekey fetch + rebuild, throttled to once per hour per JID. We were skipping this entirely, so `ensure_e2e_sessions_resolved` would see the corrupt session, return "present", and we'd re-encrypt against the same broken chain forever. Mirroring it: - `Client.session_recreate_history: Arc<Mutex<HashMap<Jid, Instant>>>` — per-peer last-recreate timestamp, gates the throttle. - `should_recreate_session(retry_count, jid)`: · no session → recreate (any retry). · session + retry<2 → no-op. · session + retry>=2 + cold or >1h history → recreate, stamp history. · session + retry>=2 + recent stamp → throttled. - In `handle_retry_receipt`, after `update_local_signal_session`, if the receipt carries no `<keys>` and the gate agrees: take the per-sender session lock, delete the local session, flush. The subsequent `ensure_e2e_sessions_resolved` then runs the WA Web prekey-fetch path and builds a fresh session via `process_prekey_bundle` (archives the corrupt one into `previous_sessions[0]`). The new path is strictly additive to the WA Web flow already in place (regId mismatch + base-key collision still trigger their own deletes first); both can fire on the same receipt without conflict.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c31f06dbbe
ℹ️ 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".
| let has_session = self | ||
| .signal_cache | ||
| .has_session(&signal_address, &*device_guard.backend) | ||
| .await | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Treating has_session errors as false causes the retry path to behave as if no session exists, which can trigger an unnecessary forced recreate/delete on transient backend failures (e.g., temporary SQLite/read errors). In that case we may drop a valid session and churn prekey fetches even though the remote state is fine; this path should distinguish "no session" from "failed to check session" and skip destructive recreation when the lookup fails.
Useful? React with 👍 / 👎.
Address review feedback on PR #635: - CodeRabbit (Major): `dm_retry_emits_enc_directly_under_message_with_recipient` reused the same JID for `to_jid` and `recipient_jid`, so a swapped-args regression (e.g. `recipient = to_jid`) wouldn't fail. Split into distinct device-vs-user JIDs and assert both attributes individually so the semantic split is pinned. - Codex (P2): `should_recreate_session` was treating `has_session` errors as `false`, mirroring "no session present" — a transient backend read failure would force an unnecessary session delete + prekey fetch. Whatsmeow returns `false` (no recreate) in this case (`retry.go:161-163`); match that and log a warning so the diagnostic isn't lost. Skipped review items already resolved on the branch: - "recipient should follow alt_chat" — landed in `62dd6f7e` via `info.recipient` (the receipt's `recipient` attr) instead of `info.chat`. - "trim verbose docstring on prepare_dm_retry_stanza" — landed in `e02fd2af`.
`resolve_retry_chat_info` populates `info.recipient` from the receipt's `recipient` attribute. The DM resend path then forwards it verbatim into `prepare_dm_retry_stanza`'s stanza, matching WA Web's `f && (k.recipient = f)`. Pre-fix the resend used `info.chat.clone()`, which silently disagreed with the original outbound's namespace whenever `take_recent_message` resolved via `alt_chat` (PN↔LID cross-namespace). Pins two invariants the structural fix relied on: - Receipt with `recipient="..."` in a different namespace than `from` → `info.recipient` carries the attr's JID, not anything derived from `info.chat`. - Receipt without `recipient` attr → `info.recipient` is `None`, so the resend stanza drops the attribute (mirrors WA Web for non-self DMs).
The actual root cause behind the prod "Android can't decrypt, bot can't decrypt" deadlock that PR #635's earlier commits didn't fully resolve. Both PDO sites (`send_pdo_placeholder_resend_request`, `fetch_message_history`) hardcoded `own_pn.to_non_ad()` as the peer target. WA Web's `WAWebSendNonMessageDataRequest` picks the namespace dynamically: u = getMePnUserOrThrow() c = u.isLid() && getMaybeMeDeviceLid() ? getMaybeMeDeviceLid() : getMeDevicePnOrThrow() For LID-1:1-migrated accounts (the only kind on prod now), the primary phone's Signal state with this device is keyed under LID. The pkmsg that `ensure_e2e_sessions` emits alongside the PDO is also our only mechanism to reset that Signal slot after divergence. Addressing it over PN rebuilt the wrong slot, so the LID-namespace ratchet the phone actually used on its outbound side stayed diverged forever — every inbound msg from the phone hit BadMac, retry receipts with fresh keys never moved the phone's LID session, and we kept accumulating sibling-side previous-state copies (up to 6+ in prod) all derived from prekey bundles that the phone's LID slot didn't reflect. `self_peer_target(&Device)` helper centralises the WA-Web choice: LID device 0 when `device.lid.is_some()`, else PN device 0, else `ClientError::NotLoggedIn`. Both PDO callers go through it now. Tests pin the three states (LID preferred, PN fallback, no-identity error).
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 `@src/retry.rs`:
- Around line 742-766: The session_recreate_history map is never pruned so it
grows unbounded; before using it in the current logic (where you lock
session_recreate_history and read/insert entries) remove stale entries whose
timestamp is older than RECREATE_SESSION_TIMEOUT: after acquiring the lock (the
same place where you get history), compute now = wacore::time::Instant::now()
and prune entries by retaining only those with now.saturating_duration_since(ts)
< RECREATE_SESSION_TIMEOUT (or use drain_filter if available) and then continue
the existing has_session/retry_count/recent logic so the map stays bounded.
🪄 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: aeed411a-eff9-482e-820b-55f8ba6efd89
📒 Files selected for processing (4)
src/client.rssrc/pdo.rssrc/retry.rswacore/src/send.rs
…Web parity)" This reverts commit d6b4612.
Root cause of the prod deadlock — the 480KB `236395184570386@lid.0`
blob that nothing could decrypt and kept getting rebuilt at 460KB+
after each manual SQLite delete.
`migrate_signal_sessions_on_lid_discovery`'s "both PN and LID slots
have a session" branch deleted PN and kept LID. That's wrong whenever
LID was the fresh stub (created by some path that ran
`process_prekey_bundle` against Bob's prekey bundle for the *new*
LID address before the migration happened) and PN held the real
Double Ratchet state the peer's outbound chain was ratcheting against.
After the delete there was no archived copy of the working session
left under either address, so every inbound `<enc type="msg">` from
Android failed BadMac; the bot's retry receipts with fresh keys
asked Android to rebuild, but Android was already keying against the
session WE no longer had, so its retransmit landed on the same dead
chain.
Whatsmeow's `MigratePNToLID` does the inverse — its SQL is
INSERT INTO whatsmeow_sessions (our_jid, their_id, session)
SELECT our_jid, replace(their_id, $2, $3), session
FROM whatsmeow_sessions
WHERE our_jid=$1 AND their_id LIKE $2 || ':%'
ON CONFLICT (our_jid, their_id) DO UPDATE SET session=excluded.session
i.e. the PN row OVERWRITES any pre-existing LID row, then PN is
dropped. That preserves the working chain in the LID address.
This commit applies the same policy on our side: collapse the two
branches and always overwrite the LID slot with the PN session
before dropping PN.
Reproduction at `src/client/lid_pn.rs::migration_preserves_working_session_when_both_namespaces_present`
fails on `main` (LID stub survives) and passes after the fix.
Also adds `wacore/libsignal/tests/session_divergence.rs` covering
nine hypotheses for the prod failure mode at the libsignal layer.
Eight pass — the architecture is fine when the right session is
under the right address. The one that's pinned as a documented
failure (`alice_delete_then_rebuild_loses_old_chain`) is exactly
the state we now avoid by not discarding PN.
…hain ROOT CAUSE of the prod deadlock for `236395184570386@lid.0`. The log signature was six archived sessions, six different alice_base_key values, and yet every single one had its `574b6be3…` receiver chain sitting at index 846 — counter 940+ from the peer, nothing decrypts. The only way independent sessions land on the *same* chain index is if the chain index moves regardless of MAC verdict. `decrypt_message_with_state` called `get_or_create_chain_key` (DH may add a new receiver chain + rotate root/sender key) and `get_or_create_message_key` (saves skipped keys + writes the next chain key via `set_receiver_chain_key`) BEFORE `verify_mac`. On MAC failure the function returned `BadMac`, but the chain advance + DH ratchet step + consumed skipped keys were already committed into `state`. The caller's `set_session_state(current_state)` then put those mutations back into the record. Net effect: every failed-MAC attempt walked the chain one step further. After ~hundreds of attempts (Android retransmitting the same broken-chain msg as Alice cycled through sessions) the chain was pinned at an index that didn't match the peer's actual root key, and the deadlock was permanent. Once one session was in this state, six rebuilds via `process_prekey_bundle` produced six fresh sessions that all walked their own chain to the same advanced index for the same reason. Fix: clone the state into a scratch copy at the top of `decrypt_message_with_state`, run all mutating helpers against the scratch, and only commit (`*state = scratch`) after both MAC and plaintext verification succeed. On failure the scratch is dropped and `state` is untouched — exactly the transactional semantics Signal's reference impl has. Reproduction at `wacore/libsignal/tests/session_divergence.rs::failed_mac_must_not_advance_receiver_chain`: warm 3 msgs (chain at 3), send 10 tampered ciphertexts back-to-back (each `verify_mac` rejects). Asserts the chain index stays at 3 through all 10 rounds. Pre-fix that test failed at round 0 — chain advanced to 4. Post-fix all 10 rounds keep the chain at 3 and the workspace test suite (746 wacore + 521 whatsapp-rust + the rest) is green. Clone cost: `SessionState` wraps a `SessionStructure` protobuf — bounded by `MAX_RECEIVER_CHAINS = 5`, each chain bounded by `MAX_MESSAGE_KEYS = 2000`. Worst-case clone is a five-deep walk over the receiver chains. Same constant-factor envelope as `session_record.serialize()` which runs on every persisted decrypt.
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/libsignal/tests/session_divergence.rs`:
- Around line 519-611: The inner helper function alice_chain_total uses
triple-slash doc comments (///) which are meant for public API documentation;
change those to normal line comments (//) within the function body and above it
so the comments are purely cosmetic and do not generate hidden docs, keeping the
function name alice_chain_total unchanged and preserving its logic.
🪄 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: fd709002-ba7a-43df-a8f3-b6b45c5b51a2
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/tests/session_divergence.rs
ROOT CAUSE the previous fixes missed. The inbound decrypt path only attempted `try_pn_to_lid_migration_decrypt` on `InvalidPreKeyId`. When a peer's outbound chain ratcheted against a session living under the PN-keyed `ProtocolAddress` and a freshly built LID-keyed session shadowed it, the LID record loaded by `signal_address` had a root key unrelated to the peer's actual chain — every msg failed BadMac, the PN-keyed blob with the working ratchet sat untouched in the backend, and the retry-receipt path never gave libsignal a chance to look at it. From there the deadlock was permanent: nothing in the prod recovery loop (PDO peer pkmsg, retry receipt with keys, base-key collision check) reroutes the working PN ratchet into the LID slot once both addresses already hold sessions. `try_pn_to_lid_migration_decrypt` is exactly the right tool — it resolves the LID user's PN via `lid_pn_cache`, runs `migrate_signal_sessions_on_lid_discovery` (PN wins on conflict per whatsmeow's `MigratePNToLID` policy that landed earlier in this PR), then retries the decrypt with the now-correctly-keyed session. Same helper already covers the InvalidPreKeyId path on session establishment from a stale PN bundle; we now reuse it before the BadMac/InvalidMessage retry receipt fires. Test fixtures: - `src/message.rs::test_badmac_migrates_pn_session_when_lid_shadow_exists` builds the exact shadow: PN-keyed session with the peer's working ratchet, LID-keyed session built from a fresh prekey bundle. A Whisper msg on the original PN ratchet arrives keyed to the LID address, fails BadMac on the LID stub, then post-fix the migration promotes the PN blob into the LID slot and decrypt succeeds. - `wacore/libsignal/tests/session_divergence.rs::pkmsg_reset_does_not_fix_peer_outbound_if_delivered_to_wrong_store_key` pins the libsignal-level invariant the bug exploits: a reset pkmsg must be delivered to the peer's actual session store key, otherwise `process_prekey` promotes the wrong record and the peer's next outbound keeps coming on the unchanged old ratchet. Whole workspace test suite green (522 whatsapp-rust + 11 session_divergence + the rest). Co-authored-by: Codex <noreply@openai.com>
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 `@src/message.rs`:
- Around line 1078-1095: The helper try_pn_to_lid_migration_decrypt currently
mutates the live LID session before proving the PN retry can decrypt, so change
its behavior to operate on a cloned/scratch session view (or a temporary
adapter/store clone) and do not write changes to the real LID ratchet; have it
return either a migration result/commit token or the decrypted payload
indicating success, and only apply/commit the migration to the real
adapter/store (and update the live LID session) after the PN retry decrypt (the
call site using parsed_message, sender_encryption_jid, signal_address, adapter,
etc.) succeeds; ensure BadMac/InvalidMessage retry paths only call the commit
when decrypt is verified.
🪄 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: 9c417971-5f84-4d13-baf0-7b11b21eef63
📒 Files selected for processing (2)
src/message.rswacore/libsignal/tests/session_divergence.rs
…arity) Last user-visible deadlock — bot kept sending PDO pkmsg to PN device 0 but Android's LID-keyed Signal slot stayed on the diverged ratchet. Three sessions deleted in prod, no recovery, peer counter kept climbing (`574b6be3…` at 940+) on the same chain. Whatsmeow's `SendPeerMessage` uses `cli.getOwnID().ToNonAD()` and `Store.GetJID()` returns the LID JID after LID-1:1 migration. So its PDO target is LID when migrated, PN otherwise. WA Web's `WAWebSendNonMessageDataRequest` still hardcodes PN via `getMeDevicePnOrThrow_DO_NOT_USE()` — the "DO_NOT_USE" suffix flags it as un-migrated tech debt the codebase hasn't moved off yet, and empirically that path leaves the LID slot stranded on the old ratchet. This crate previously matched WA Web (PN target). After repeated prod cycles where the bot's pkmsg landed on PN, never refreshed Android's LID-keyed outbound chain, and the bot stayed permanently unable to decrypt the peer, switching to whatsmeow's policy is the right move. The user has accepted whatsmeow parity throughout this PR for similar gaps (`should_recreate_session`, `MigratePNToLID`). `self_peer_target(&Device)` picks the namespace: LID device 0 when `device.lid.is_some()`, PN device 0 otherwise, typed `ClientError::NotLoggedIn` if neither is set. Both PDO callers (`send_pdo_placeholder_resend_request`, `fetch_message_history`) route through it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234b922404
ℹ️ 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 peer-message destination = our primary phone, in the namespace | ||
| /// the phone actually keys its Signal store under. Mirrors whatsmeow's | ||
| /// `SendPeerMessage` → `cli.getOwnID().ToNonAD()`: `Store.GetJID()` | ||
| /// returns the LID JID once the bot is LID-migrated. | ||
| /// |
There was a problem hiding this comment.
The repository guideline in AGENTS.md requires code comments to be concise and explain only why, but this newly added block is a long implementation narrative (including historical behavior and protocol walkthrough) that documents what the code does. Keeping comments at this level of detail makes maintenance harder and conflicts with the project’s required review convention, so this should be reduced to a short why-focused note.
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pdo.rs`:
- Around line 35-49: Trim the lengthy rationale comment starting with "Self
peer-message destination = our primary phone..." (and similar long blocks around
the other noted sites) to a short, focused explanation of the local invariant:
state that the peer destination prefers LID when present and PN otherwise and
why that matters for session refresh; remove the upstream implementation
history, naming archaeology, and example code paths from the inline comment, and
move those detailed explanations into the PR description or a separate
design/test note.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…parity)
When the bot sends a peer message that's a pkmsg, the phone's
Signal stack expects the ADVSignedDeviceIdentity blob in a
`<device-identity>` element next to `<enc>` so it can verify the
linked device's identity before promoting the new session.
Without that element the phone ack's the stanza (we see
`<ack class="message">` for every PDO id) but skips the session
promotion path entirely. Its outbound ratchet stays on the
diverged chain (`574b6be3…` in prod, counter climbing past 990
in the latest log) and the bot's inbound side never recovers
even with every other fix in this PR (transactional decrypt,
PN→LID migration on BadMac, LID-target PDO, etc.) wired in.
Whatsmeow's `preparePeerMessageNode` builds:
[<meta appdata="default"/>, <enc>, <device-identity>?]
The `<meta appdata="default"/>` element is always included; the
`<device-identity>` element is added only on the pkmsg path
(`isPreKey && MessengerConfig == nil`).
Mirror that. `prepare_peer_stanza` now takes
`account: Option<&AdvSignedDeviceIdentity>` so callers in
`src/send.rs` thread the bot's stored device account into the
peer-stanza builder. The element is `device-identity` with the
proto-encoded `account` as content.
This is the missing piece behind the deadlock the user reported
across the last ~6 logs: pkmsgs were physically reaching Android
(server ack confirms delivery) but Android's process_prekey was
silently rejecting/ignoring them for lacking the identity proof
chain it needs for pkmsg-driven session promotion. With the
element present, Android's next outbound after our PDO should
land on a fresh ratchet (counter 0 on a new sender key) and
the bot's existing fresh session decrypts it cleanly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a54365f6a
ℹ️ 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".
| signal_address: &ProtocolAddress, | ||
| message: &wa::Message, | ||
| request_id: String, | ||
| account: Option<&wa::AdvSignedDeviceIdentity>, |
There was a problem hiding this comment.
Adding the new account parameter to prepare_peer_stanza without updating all in-repo call sites breaks benchmark builds: cargo check -p wacore --benches now fails with E0061 at wacore/benches/send_receive_benchmark.rs:666 because the call still passes 6 args. This makes cargo bench unusable for performance validation until either the signature is made backward-compatible or every caller is updated in the same change.
Useful? React with 👍 / 👎.
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)
961-968:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThis signature bump is still breaking the build.
Line 968 adds a required
accountparameter, and CI already showssend_receive_benchmark.rsstill callingprepare_peer_stanzawith six arguments. Update the remaining call site(s) in this PR or the repo stays uncompilable.🤖 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 `@wacore/src/send.rs` around lines 961 - 968, The build breaks because prepare_peer_stanza was changed to require a seventh parameter account: Option<&wa::AdvSignedDeviceIdentity>; update every call site (e.g., the failing call in send_receive_benchmark.rs) to pass an appropriate Option<&wa::AdvSignedDeviceIdentity> (pass Some(&account_value) where you have an AdvSignedDeviceIdentity in scope, or None if not available), ensuring the argument type matches Option<&wa::AdvSignedDeviceIdentity> and the order remains: session_store, identity_store, transport_jid, signal_address, message, request_id, account when invoking prepare_peer_stanza.
🤖 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/send.rs`:
- Around line 987-995: The comment in wacore/src/send.rs includes a hardcoded
production JID ("236395184570386@lid.0") and an explicit "prod" label; remove
that sensitive identifier and replace it with a sanitized placeholder or
internal incident reference (e.g., "<REDACTED_JID>" or "INCIDENT-XXXX") in the
comment that explains preparePeerMessageNode and
device-identity/ADVSignedDeviceIdentity, keeping the technical explanation
intact; update any nearby comment text that says "prod" to a neutral term like
"production" or the incident reference to avoid embedding user-identifying data.
---
Outside diff comments:
In `@wacore/src/send.rs`:
- Around line 961-968: The build breaks because prepare_peer_stanza was changed
to require a seventh parameter account: Option<&wa::AdvSignedDeviceIdentity>;
update every call site (e.g., the failing call in send_receive_benchmark.rs) to
pass an appropriate Option<&wa::AdvSignedDeviceIdentity> (pass
Some(&account_value) where you have an AdvSignedDeviceIdentity in scope, or None
if not available), ensuring the argument type matches
Option<&wa::AdvSignedDeviceIdentity> and the order remains: session_store,
identity_store, transport_jid, signal_address, message, request_id, account when
invoking prepare_peer_stanza.
🪄 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: b5301183-62aa-406a-afc7-6d9462a5f21b
📒 Files selected for processing (2)
src/send.rswacore/src/send.rs
Pin the three-element layout whatsmeow's `preparePeerMessageNode` emits and the bug fix in 8a54365 restored: [<meta appdata="default"/>, <enc type="pkmsg">, <device-identity>]. Without the `<device-identity>` element on the pkmsg path the primary phone ack'd peer messages at the XMPP layer but its Signal stack silently rejected the pkmsg, leaving its outbound ratchet on the diverged chain and the linked device permanently stuck on BadMac decryption. Prod recovery confirmed (Android↔bot session re-synced within 3s of the redeploy carrying the element). - `peer_pkmsg_includes_meta_and_device_identity` pins the layout + asserts `device-identity` carries a non-empty proto. - `peer_pkmsg_omits_device_identity_when_account_missing` covers the pre-pairing edge — graceful omission instead of panic.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/lid_pn.rs (1)
351-380:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftTake the per-sender session locks before doing PN→LID rewrites.
This path is mutating Signal session state straight through
signal_cache, and both persist paths can hit it while send/decrypt is ratcheting the same peer. Line 379-380 does a copy-then-delete withoutsession_locks, so a concurrent ratchet can get dropped or a newer LID session can get clobbered by the stale PN snapshot. Lock both addresses in a stable order around the whole migration sequence.As per coding guidelines, "Use
session_locksto serialize per-sender Signal encrypt/decrypt operations andmessage_enqueue_locksto serialize per-chat incoming message processing; outgoing sends are not per-chat locked".🤖 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/lid_pn.rs` around lines 351 - 380, migrate_signal_sessions_on_lid_discovery currently copies then deletes Signal sessions via signal_cache.get_session / put_session / delete_session without per-sender locking; before touching sessions acquire session_locks for both pn_proto and lid_proto in a stable order (e.g., sort the two protocol addresses) and hold the locks for the entire read-modify-write sequence, then perform get_session, put_session, delete_session while locked and finally release the locks; reference the migrate_signal_sessions_on_lid_discovery function and the signal_cache.get_session / put_session / delete_session calls and use the existing session_locks mechanism to serialize these operations.
🤖 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.
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 351-380: migrate_signal_sessions_on_lid_discovery currently copies
then deletes Signal sessions via signal_cache.get_session / put_session /
delete_session without per-sender locking; before touching sessions acquire
session_locks for both pn_proto and lid_proto in a stable order (e.g., sort the
two protocol addresses) and hold the locks for the entire read-modify-write
sequence, then perform get_session, put_session, delete_session while locked and
finally release the locks; reference the
migrate_signal_sessions_on_lid_discovery function and the
signal_cache.get_session / put_session / delete_session calls and use the
existing session_locks mechanism to serialize these operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef76984d-795a-495a-86f8-799f843bac56
📒 Files selected for processing (6)
src/client/lid_pn.rssrc/retry.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/tests/session_divergence.rswacore/src/send.rs
- prepare_peer_stanza: pre-flight check before message_encrypt so a missing account doesn't burn the sender ratchet (CodeRabbit). Uses load_session + unacknowledged_pre_key_message_items to catch both "no session" and "session with un-acked pre-key" — both produce pkmsg. - New test peer_pkmsg_preflight_no_ratchet_burn_without_session asserts the store has no session after the failed call. - peer_pkmsg_errors_when_account_missing now serializes the SessionRecord before and after the failing call and asserts equality (Codex: a chain-index check alone misses partial restores). - failed_mac_must_not_advance_receiver_chain: same byte-level compare added on top of the chain-index check. - bench establish_bidirectional: round-trip b->a to clear a's pending_pre_key so the bench's None-account path stays valid. - retry should_recreate_session: lazy prune (threshold 256) instead of scanning on every call under the global Mutex. - Trim narrative comments in lid_pn.rs, retry.rs, session_divergence.rs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 676e07c7aa
ℹ️ 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 history.contains_key(jid) { | ||
| return None; |
There was a problem hiding this comment.
should_recreate_session currently suppresses all future recreates for a JID as soon as it appears in session_recreate_history (history.contains_key(jid)), but there is no per-JID age check at that decision point. Because pruning only runs when the map grows past 256 entries, environments with fewer active peers will never evict old entries, so the intended “allow again after 1 hour” behavior never happens and a peer can remain permanently unrecoverable after the first forced recreate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/retry.rs (1)
761-770:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLook, this throttle logic has a bug when the map is under the prune threshold.
When
history.len() <= 256, pruning is skipped (lines 751-754). But at line 765,contains_key(jid)returns true for entries older thanRECREATE_SESSION_TIMEOUT— those stale entries should've been removed. This incorrectly throttles recreates that should be allowed.The check needs to verify the timestamp, not just existence:
- if history.contains_key(jid) { - return None; - } + let recent = history + .get(jid) + .copied() + .is_some_and(|prev| now.saturating_duration_since(prev) < RECREATE_SESSION_TIMEOUT); + if recent { + return None; + }This way we actually check whether the entry is within the throttle window, not just whether it exists at all.
🤖 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/retry.rs` around lines 761 - 770, The current throttle uses history.contains_key(jid) which wrongly blocks entries that exist but are stale; instead fetch the timestamp with history.get(jid), compare it against now and RECREATE_SESSION_TIMEOUT (i.e. if stored_time + RECREATE_SESSION_TIMEOUT > now then treat as throttled), otherwise treat as absent, update/insert history.insert(jid.clone(), now) and allow recreation; keep the existing MIN_RETRY_FOR_BASE_KEY_CHECK check and ensure you overwrite stale entries when permitting a recreate.
🤖 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.
Outside diff comments:
In `@src/retry.rs`:
- Around line 761-770: The current throttle uses history.contains_key(jid) which
wrongly blocks entries that exist but are stale; instead fetch the timestamp
with history.get(jid), compare it against now and RECREATE_SESSION_TIMEOUT (i.e.
if stored_time + RECREATE_SESSION_TIMEOUT > now then treat as throttled),
otherwise treat as absent, update/insert history.insert(jid.clone(), now) and
allow recreation; keep the existing MIN_RETRY_FOR_BASE_KEY_CHECK check and
ensure you overwrite stale entries when permitting a recreate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fae2dba9-982c-4e8a-bb2a-264eb80d70d1
📒 Files selected for processing (5)
src/client/lid_pn.rssrc/retry.rswacore/benches/send_receive_benchmark.rswacore/libsignal/tests/session_divergence.rswacore/src/send.rs
CodeRabbit P1 — migrate_signal_sessions_on_lid_discovery now acquires the per-address session_lock for both pn_proto and lid_proto in stable lexicographic order before the read-modify-write cycle. Without the locks a concurrent message_encrypt on LID would race with the migrated put and either clobber the migration or read mid-update state. Stable order avoids deadlock with the (always-one-lock) encrypt/decrypt callers. New test migration_blocks_on_per_address_session_lock holds the LID lock externally and asserts migration blocks until release. Codex P1 — should_recreate_session previously skipped recreates via history.contains_key(jid) alone. With lazy pruning (threshold 256) the prior retain() that made contains_key imply "within window" no longer runs in low-traffic deployments, so an expired entry stayed valid forever and pinned the peer. Added explicit age check at the decision site. wacore::time::Instant: Sub<Duration> impl mirroring the existing Add.
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 `@src/retry.rs`:
- Around line 2019-2024: The test currently avoids exercising the "entry past
the throttle window allows recreate" branch; update the test that verifies
recreate throttling to seed an expired entry by inserting an entry timestamped
with wacore::time::Instant::now() - (throttle_duration + small_epsilon) (using
the new Instant - Duration support), then call should_recreate_session(...) and
assert it returns true so the expired-history path is covered; ensure you use
the same key/peer used by the implementation and clean up or override the map
entry insertion helper the test uses so the entry is considered expired by
should_recreate_session.
🪄 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: 9596a84f-ec24-40d7-8333-590f45af5daf
📒 Files selected for processing (3)
src/client/lid_pn.rssrc/retry.rswacore/src/time.rs
Past-stamp via Instant - Duration saturates to 0 in young test runtimes, so the assertion still falls into the "throttled" branch. Refactored should_recreate_session to delegate to should_recreate_session_at(now), and the matrix test now passes a future `now` (stamp + TIMEOUT + 1s) to hit the expired-entry path. Codex-flagged: the age-check at the decision site is load-bearing in low-traffic deployments where lazy pruning doesn't fire, so it needs a regression test.
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 `@src/retry.rs`:
- Around line 2032-2047: Update the inline comment numbering to match the
surrounding test steps: change the "// 5)" comment above the injectable-clock
block to "// 4)" and rename the following "// 4) no session" comment to "// 5)".
This keeps the sequence consistent around the test that computes stamp_then,
uses RECREATE_SESSION_TIMEOUT and calls client.should_recreate_session_at(...).
🪄 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: b77f1d6c-88f9-4886-9406-f2ab2e498340
📒 Files selected for processing (1)
src/retry.rs
E2E test_pn_only_session_causes_undecryptable_on_lid_lookup deadlocked after the previous commit added session_locks to the migration loop. decrypt_message holds session_lock_for(<lid_addr>) when it calls try_pn_to_lid_migration_decrypt, and async_lock::Mutex is not reentrant, so the migration's per-device LID lock acquisition for the same device hangs forever. Added migrate_signal_sessions_on_lid_discovery_with_held_lock that takes the held device id and skips the LID lock for that specific device (still acquires PN). All other call sites use the original no-held-lock entrypoint and are unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c643592f41
ℹ️ 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".
| } else { | ||
| let second_lock = self.session_lock_for(&second_key).await; | ||
| Some(second_lock.lock_arc().await) |
There was a problem hiding this comment.
migrate_signal_sessions_on_lid_discovery now takes per-address session locks, but this function is invoked from process_session_enc_batch while that path already holds session_lock_for(sender_encryption_jid) for decrypt. When the sender is LID (the only case that triggers migration), the loop over 0..=99 includes the same address/device and this code attempts to lock it again (first_lock or second_lock), which awaits forever on a non-reentrant mutex. In practice, a BadMac/InvalidPreKey/SessionNotFound recovery attempt can deadlock the decrypt task and stall further messages for that sender.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f299faa7
ℹ️ 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".
| let (_pn_guard_opt, _lid_guard_opt) = if skip_lid { | ||
| (Some(pn_lock.lock_arc().await), None) | ||
| } else { |
There was a problem hiding this comment.
When migrate_signal_sessions_on_lid_discovery_with_held_lock is used from decrypt, the caller already holds the LID session lock, but this branch then acquires PN directly; concurrently, a normal migration call can take PN -> LID when pn_key <= lid_key, creating an ABBA cycle (decrypt: LID -> PN vs migration: PN -> LID) that can deadlock message decryption for that peer. To avoid this, the held-lock variant should not wait on PN while another path can still wait on LID after PN in the same function.
Useful? React with 👍 / 👎.
Previously the decrypt path passed `held_device_id: u16` to a `_with_held_lock` variant so the migration could skip re-acquiring one specific LID device's lock. That coupled caller and callee — caller had to track which device's lock it held, callee had to special-case skip. New design: `try_pn_to_lid_migration_decrypt` takes `&Arc<Mutex>, &mut Option<MutexGuardArc>` and manages the drop → migrate → reacquire dance internally. Migration is fully self-contained again (acquires both PN and LID per-device locks in stable order, no skip parameter). Caller's `session_guard` is replaced via `*session_guard = Some(session_mutex.lock_arc().await)` so subsequent payloads in the batch stay serialized. Allocations: dropped two `to_string()`s per device iteration in the migration loop by using `ProtocolAddress::as_str()` for the lock keys and the ordering comparison (100 iterations × 2 = 200 fewer allocations per migration). Regression test `migration_lock_dance_completes_when_caller_drops_guard` mirrors the exact production pattern (hold → drop → migrate → reacquire) and asserts it never deadlocks. Existing `migration_blocks_on_per_address_session_lock` still pins the locking invariant for non-decrypt callers.
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 `@src/message.rs`:
- Around line 1457-1464: The current release/reacquire of *session_guard around
migrate_signal_sessions_on_lid_discovery(...) creates a window that breaks
per-sender serialization; fix by eliminating the handoff: either (A) keep the
sender lock held across migration and retry decrypt by making
migrate_signal_sessions_on_lid_discovery non-reentrant (refactor it to not call
back into session_lock_for/session_mutex), or (B) replace the non-reentrant
async_lock used by session_mutex/session_lock_for with a reentrant primitive so
the migration loop can safely re-enter while the caller retains the guard;
specifically, change the code around session_guard,
migrate_signal_sessions_on_lid_discovery, and session_mutex.lock_arc() so
migration work does not require releasing *session_guard (or make migration
collect what it needs up-front and run deferred without touching per-sender
locks).
🪄 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: 42d90388-a9a9-4a82-8a52-410aa7c702f1
📒 Files selected for processing (3)
src/client/lid_pn.rssrc/message.rssrc/retry.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 668c385606
ℹ️ 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".
| let needs_account = match session_store.load_session(signal_address).await? { | ||
| None => true, |
There was a problem hiding this comment.
Use of session_store.load_session here is a destructive read for the production SessionAdapter (it checks out the record from SignalStoreCache until store_session returns it). Because this preflight path never stores the record back, the subsequent message_encrypt call can observe SessionNotFound (or behave as if no session exists) when account is None, even if a valid session was present. This breaks peer sends in that fallback path and can leave the cache slot checked out; switch this preflight check to a non-destructive session probe or restore the loaded session before encrypting.
Useful? React with 👍 / 👎.
Critical: - message_decrypt_prekey: snapshot the SessionRecord before process_prekey and restore it on inner decrypt failure. Pre-fix, a tampered pkmsg with a valid prekey header would `promote_state` a new session, then BadMac on the inner SignalMessage; the partially-built session was still persisted via store_session, replacing current_session with one only the attacker could write to. New regression test pkmsg_decrypt_failure_does_not_persist_promoted_session reconstructs a pkmsg with tampered inner bytes (header intact so process_prekey accepts it) and asserts Bob's store stays empty after BadMac. Defensive fixes (symmetric to peer pkmsg pre-flight from 5a70089): - prepare_dm_retry_stanza now has the same load_session + unacknowledged_pre_key_message_items pre-flight as prepare_peer_stanza; refuses to ship pkmsg DM retry without <device-identity> before message_encrypt burns the sender chain. Asserive check after encrypt upgraded from `if let Some(acc)` to `account.ok_or_else(...)`. Three existing DM retry tests updated to pass pkmsg_account_proto() (they were implicitly relying on the silent-omit behavior). New test dm_retry_pkmsg_preflight_errors_when_account_missing pins the byte-identical session invariant after the failed call. - retry.rs: gate the "no bundle + regId mismatch → delete session" branch on `!keys_node_present`. A rejected key bundle (peer reg-ID change security refusal, parse errors) no longer falls through to destructive session deletion as a side effect; we log+skip instead. - migration loop: hoisted the magic `0..=99u16` to a named constant MIGRATION_DEVICE_RANGE and clarified the identity-vs-session policy asymmetry (LID-wins for identity is intentional: peer re-pair would put the fresh identity on the namespace we're migrating to). - retry.rs: comment on getTargetChat path 3 now reflects what the code actually does (fall back with warn) rather than WA Web's strict abort+null, since the test pinning the fallback shipped with #88e4808. Perf: - migrate_signal_sessions_on_lid_discovery: replaced `pn.to_string()` / `lid.to_string()` with the borrowed `&str` that Jid::pn_device / lid_device already accept (impl Into<CompactString>, inline for ≤24-byte user parts). Saves 200 allocations per migration. Deferred (pre-existing, out of this PR's scope, documented): - The exact "WA Web abort on peer device retry without recipient" change would alter receipt handling beyond this PR's deadlock fix. - magic 0..=99 device range — named constant added but bound itself unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f96830ba
ℹ️ 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".
| // advanced chain. Otherwise we'd burn the ratchet for a stanza the | ||
| // peer's Signal layer can't promote. | ||
| if account.is_none() { | ||
| let needs_account = match session_store.load_session(&signal_address).await? { |
There was a problem hiding this comment.
prepare_dm_retry_stanza uses session_store.load_session for an inspection-only preflight when account.is_none(), but in production SessionAdapter::load_session goes through SignalStoreCache::get_session, which checks the record out and marks it CheckedOut until store_session is called. This preflight path never stores the record back before calling message_encrypt, so in the "account missing + existing session" case the subsequent encrypt sees SessionNotFound and the checked-out slot can remain stranded, causing retry sends to fail even though a valid session existed.
Useful? React with 👍 / 👎.
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/lid_pn.rs (1)
421-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat LID identity read failures as "missing".
This path currently collapses
ErrandOk(None)together, so a transientget_identity(&lid_proto, ...)failure can overwrite a fresher LID identity with the PN copy and then delete the PN slot. That breaks the LID-wins policy you documented right above it.Suggested fix
- if self - .signal_cache - .get_identity(&lid_proto, backend.as_ref()) - .await - .ok() - .flatten() - .is_none() - { - self.signal_cache - .put_identity(&lid_proto, &identity_data) - .await; - info!("Migrated identity {} -> {}", pn_proto, lid_proto); - } - self.signal_cache.delete_identity(&pn_proto).await; + match self + .signal_cache + .get_identity(&lid_proto, backend.as_ref()) + .await + { + Ok(None) => { + self.signal_cache + .put_identity(&lid_proto, &identity_data) + .await; + info!("Migrated identity {} -> {}", pn_proto, lid_proto); + self.signal_cache.delete_identity(&pn_proto).await; + } + Ok(Some(_)) => { + self.signal_cache.delete_identity(&pn_proto).await; + } + Err(e) => { + warn!( + "Skipping identity migration {} -> {}: failed to read LID identity: {e:?}", + pn_proto, lid_proto + ); + } + }🤖 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/lid_pn.rs` around lines 421 - 434, The code currently treats get_identity(...).await errors the same as Ok(None), risking overwriting a valid LID; change the logic to explicitly match the Result from self.signal_cache.get_identity(&lid_proto, backend.as_ref()).await: if Ok(Some(_)) then do nothing, if Ok(None) then call self.signal_cache.put_identity(&lid_proto, &identity_data).await and then info! migrate, and if Err(e) then log the error and skip migration (do not call delete_identity or put_identity). Ensure delete_identity(&pn_proto) is only executed when the migration actually succeeded (i.e., after Ok(None) -> put_identity completed).
🤖 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/send.rs`:
- Around line 1060-1080: Extract the duplicated pkmsg/account pre-flight
predicate and fail-fast logic into a single async helper (e.g.,
needs_account_for_pkmsg or ensure_account_for_pkmsg) and call it from both
send.rs and prepare_peer_stanza so both paths share the exact same behavior; the
helper should take the session_store and signal_address, perform
session_store.load_session(...).await?, inspect
record.session_state().and_then(|st|
st.unacknowledged_pre_key_message_items().ok().flatten()).is_some() to determine
the need for an account, and return a Result/boolean so the caller can bail with
the same error text when account.is_none() before calling message_encrypt,
preserving the original bail message and semantics.
---
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 421-434: The code currently treats get_identity(...).await errors
the same as Ok(None), risking overwriting a valid LID; change the logic to
explicitly match the Result from self.signal_cache.get_identity(&lid_proto,
backend.as_ref()).await: if Ok(Some(_)) then do nothing, if Ok(None) then call
self.signal_cache.put_identity(&lid_proto, &identity_data).await and then info!
migrate, and if Err(e) then log the error and skip migration (do not call
delete_identity or put_identity). Ensure delete_identity(&pn_proto) is only
executed when the migration actually succeeded (i.e., after Ok(None) ->
put_identity completed).
🪄 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: 9d4aa243-2caf-467e-9e9b-caaa3344cb21
📒 Files selected for processing (6)
src/client/lid_pn.rssrc/message.rssrc/retry.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/tests/session_divergence.rswacore/src/send.rs
…t P2) `SessionStore::load_session` is take-semantics in production: `SessionAdapter::load_session` → `SignalStoreCache::get_session` marks the slot `CheckedOut` until the caller puts it back via `store_session`. The pre-flight in `prepare_peer_stanza` / `prepare_dm_retry_stanza` loaded the record for inspection but never restored it, so a subsequent `message_encrypt` saw `Ok(None)` (treated as no session) — every retry to the same address after a passing pre-flight broke until the cache was evicted. Extracted the load+inspect+restore dance into `pkmsg_would_be_emitted` and reused it from both stanza builders. The helper restores the record on both the bail path (account.is_none() + pkmsg pending) and the pass path so the next `message_encrypt` finds the slot Present. Regression test `preflight_restores_session_with_take_store_semantics` uses an interior-mutability mock that mirrors production take-semantics (absent in `MemSessionStore`) and asserts both branches put the slot back. Also (CodeRabbit P2 outside-diff in lid_pn.rs): identity migration no longer collapses `Err` and `Ok(None)` on the LID lookup. A transient read failure now logs+skips instead of overwriting a potentially-valid LID identity with the PN copy and deleting the PN slot.
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/lid_pn.rs (1)
148-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCoalesce duplicate phone numbers before you mark
is_new.If one batch contains the same
phone_numbertwice, the first tuple can trigger migration to one LID and a later tuple can still overwrite the cache/DB mapping to a different LID. Now the final mapping points at one namespace while the migrated Signal/device state lives under another one. Dedup byphone_numberfirst, or computeis_newfrom the last occurrence only.🤖 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/lid_pn.rs` around lines 148 - 157, The loop over mappings can produce incorrect is_new flags when the same phone_number appears multiple times; before calling lid_pn_cache.get_current_lid and pushing is_new into is_new_flags you must coalesce duplicates by phone_number (or compute is_new only for the last occurrence). Change the logic that iterates over mappings (the block that uses mappings, LidPnEntry::new, lid_pn_cache.get_current_lid, lid_pn_cache.add, and is_new_flags) so that you first deduplicate mappings by phone_number (keeping final lid for that phone) or scan from the end to determine is_new only for the final occurrence, then create entries and call lid_pn_cache.add and push a single is_new per phone_number.
🤖 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/send.rs`:
- Around line 979-984: The current computation of needs_pkmsg silently treats
any error from record.session_state() or
st.unacknowledged_pre_key_message_items() as false, which lets
prepare_peer_stanza proceed into message_encrypt on failures; change this to
conservatively treat read errors as needs_pkmsg = true (or propagate the error)
by explicitly matching errors instead of using ok().flatten(): call
record.session_state() and handle Err(_) by setting needs_pkmsg = true (or
returning Err) and only treat the Some(...) -> is_some() result as false when it
successfully returns No items; mirror the error-handling approach used in
store_session so failures in the read path do not allow message_encrypt to run
with potentially invalid ratchet state.
---
Outside diff comments:
In `@src/client/lid_pn.rs`:
- Around line 148-157: The loop over mappings can produce incorrect is_new flags
when the same phone_number appears multiple times; before calling
lid_pn_cache.get_current_lid and pushing is_new into is_new_flags you must
coalesce duplicates by phone_number (or compute is_new only for the last
occurrence). Change the logic that iterates over mappings (the block that uses
mappings, LidPnEntry::new, lid_pn_cache.get_current_lid, lid_pn_cache.add, and
is_new_flags) so that you first deduplicate mappings by phone_number (keeping
final lid for that phone) or scan from the end to determine is_new only for the
final occurrence, then create entries and call lid_pn_cache.add and push a
single is_new per phone_number.
🪄 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: 61cdbc43-896a-4c95-9cea-93ece336a50a
📒 Files selected for processing (2)
src/client/lid_pn.rswacore/src/send.rs
…Rabbit) Pre-flight (wacore/src/send.rs): pkmsg_would_be_emitted previously used `.ok().flatten().is_some()` on `unacknowledged_pre_key_message_items()`. That collapses `Err(_)` and `Ok(None)` to the same "no pkmsg pending" branch, letting a corrupt session through into message_encrypt where it could burn the sender chain. Replaced with an explicit match: Err and missing session_state both conservatively return `needs_pkmsg = true` so the caller bails before encrypting. Batch dedup (src/client/lid_pn.rs): learn_lid_pn_mappings_batch iterated raw mappings and computed is_new_flags per occurrence. Duplicate phone_numbers in a single batch produced is_new=true for the first (lid_A) and is_new=false for the rest, so signal migration ran toward lid_A while the persisted mapping pointed at lid_B (the final occurrence). Now we collapse duplicates via HashMap (last lid wins) before issuing migrations. New test test_learn_lid_pn_mappings_batch_dedups_ duplicate_phones asserts the final lid is what the cache resolves to.
|
@claude review |
|
Claude finished @jlucaso1's task in 5m 9s —— View job Review: fix/dm-retry-stanza-format
Thorough PR — 8 layered fixes, each with a falsifiable regression test, and prod-verified. The automated reviewers caught real issues over several rounds and the code addressed them well. Here's my independent assessment. One issue worth addressing
Both Looks goodTransactional decrypt ( Session recreation throttle ( PN→LID migration locking ( PN-wins-on-conflict ( Identity migration error handling ( Batch dedup ( pkmsg pre-flight + session restoration ( Lock release window in migration decrypt ( Minor nits (non-blocking)
Test coverageExcellent. The regression tests are well-designed:
SummaryThis is a strong PR that methodically fixes a complex, layered production issue. The code quality is high, the test coverage is thorough, and the automated review findings have been addressed across the 26 commits. The one substantive finding is the missing pkmsg pre-flight in | |
Same bug-class the PR fixed for peer (5a70089) and dm_retry (c5f9683): prepare_group_retry_stanza called message_encrypt before checking account, and the device-identity push was gated on `if is_prekey && let Some(acc) = account` — silently omitting it when account is None. A group retry to a participant whose session was just rebuilt (pkmsg path) without an AdvSignedDeviceIdentity would burn the sender chain and ship a stanza the receiver's Signal layer can't promote. Added the same `pkmsg_would_be_emitted` pre-flight + assertive `account.ok_or_else(...)` after encrypt. Three existing group_retry tests updated to pass pkmsg_account_proto(); pkmsg_no_account renamed to group_retry_pkmsg_with_account_emits_device_identity and asserts the element is present. New test group_retry_pkmsg_preflight_errors_when_account_missing pins the byte-identical session invariant after the bail path. Flagged by claude-review bot — symmetric defense across all three preparation paths now.
- should_recreate_session_matrix: renumber the throttle-expiry scenario from 5 to 4 and the no-session scenario from 4 to 5 so the steps now read 1 → 2 → 3 → 4 → 5 in file order (claude-review). - session_divergence::failed_mac_must_not_advance_receiver_chain: the inner `alice_chain_total` helper used `///` doc comments which would generate docs for a non-pub item nested in a test fn. Switched to `//` line comments.
examples/benchmark.rs previously printed/silently dropped Pairing QR codes, so pointing it at the e2e mock server (which has no phone UI) required a manual workaround. Mirror the QR-autoresponder pattern from tests/e2e/src/lib.rs::spawn_qr_autoresponder_http: derive the admin endpoint from the ws[s] URL (host:port + /admin/mock-phone/scan-qr) and POST the QR code from inside the existing on_event closure. Behavior: - WS URL is ws:// or wss:// → derive admin URL, POST the QR on receipt. Mock server admin endpoint accepts it and pairing proceeds. - Real WhatsApp (or non-ws URL) → no admin URL derived; the closure logs the code so a phone can scan manually. Also accepts MOCK_SERVER_URL as a fallback for WHATSAPP_WS_URL so it matches the env-var convention the e2e suite uses.
…ery + peer pkmsg identity) (oxidezap#635)
1 participant
Summary
Multiple layered fixes for the self-DM / linked-device decryption deadlock that pinned
236395184570386@lid.0in prod to a stale ratchet (574b6be3…counter 940+). Confirmed end-to-end recovery in prod within ~3 seconds of deploying the last commit — Android re-synced sessions and the bot's been decrypting cleanly since.The investigation peeled back ~8 distinct issues. Each commit fixes one and includes a falsifiable regression test.
Layered fixes
1. DM retry stanza shape (
cda07efd)prepare_dm_retry_stanzawas emitting the fanout-shaped envelope (<message><participants><to jid=…><enc>) for single-device retries. Server rejected every resend with<ack error="479"/>(SmaxInvalid). WA Web'sWAWebSendMsgCreateDeviceStanza.createUserDeviceMsgStanzaputs<enc>directly under<message>plus arecipientattribute.2. Receipt's
recipientforwarded verbatim (62dd6f7e)recipient_jidon the resend was derived frominfo.chat— wrong on the alt PN↔LID namespace path. WA Web'sWAWebHandleRetryRequest:f && (k.recipient = f)— propagate the receipt'srecipientattribute as-is.3.
should_recreate_sessionwhatsmeow parity (c31f06db)When a retry receipt arrives without
<keys>for a peer we already have a session with, force a session recreate (fresh prekey fetch +process_prekey_bundle) whenretryCount ≥ 2and >1h since the last recreate for that JID. Mirrors whatsmeow'sretry.go:158-175.4. PN session wins on LID migration conflict (
712f5e49)migrate_signal_sessions_on_lid_discoverydeleted PN when both PN and LID slots had sessions — but the LID side was often the freshly built stub, not the working ratchet. Switched to whatsmeow'sMigratePNToLIDpolicy:ON CONFLICT … session=excluded.session— PN overwrites the LID stub before being dropped.5. Transactional decrypt (
bf691f5a)decrypt_message_with_stateadvanced the receiver chain viaset_receiver_chain_keyBEFOREverify_mac. Each MAC failure committed the chain step regardless, so a string of failed attempts walked the chain past the peer's actual position; from there the deadlock was permanent. Now clones into a scratchSessionState, mutates the scratch, commits*state = scratchonly after MAC + plaintext both verify.6. PN→LID migration on inbound BadMac (
53c98e05)The inbound decrypt path only attempted
try_pn_to_lid_migration_decryptonInvalidPreKeyId. When a fresh LID-keyed session shadowed the working PN-keyed session, BadMac fired forever without ever migrating the PN blob into the LID slot. Extended the migration attempt to the BadMac/InvalidMessage path too.7. PDO target is LID when LID-migrated (
234b9224)PDO peer messages were addressed to PN (matching WA Web's
getMeDevicePnOrThrow_DO_NOT_USE— the "DO_NOT_USE" suffix flags it as un-migrated tech debt). The phone's post-migration Signal store keys by LID, so PN-addressed pkmsg landed in the wrong slot and never refreshed its outbound chain. Switched to whatsmeow's policy viaStore.GetJID().ToNonAD()— LID when known.8. Peer pkmsg
<meta>+<device-identity>(8a54365f) ← the actual unblockThe fix that finally restored prod.
prepare_peer_stanzawas emitting only<enc>inside<message category="peer">. Whatsmeow'spreparePeerMessageNodebuilds[<meta appdata="default"/>, <enc>, <device-identity>?]— the<device-identity>element (the proto-encodedADVSignedDeviceIdentity) is what the primary phone uses to verify the linked device's identity before promoting a Signal session on the pkmsg path. Without it the phone XMPP-acks the stanza but the Signal layer silently rejects the pkmsg, its outbound ratchet stays on the diverged chain, and the linked device never catches up.Verification
cargo fmt --all,cargo clippy --workspace --testsclean.cargo test --workspace --exclude e2e-testsgreen (522 whatsapp-rust + 11 session_divergence + 748 wacore + the rest).dm_retry_emits_enc_directly_under_message_with_recipient— retry stanza shape.resolve_retry_chat_info_forwards_recipient_attribute_verbatim— receiptrecipientpropagation incl. alt-namespace.should_recreate_session_matrix— the four-state transition table for the throttle.migration_preserves_working_session_when_both_namespaces_present— PN wins on conflict.failed_mac_must_not_advance_receiver_chain— 10 tampered ciphertexts, chain index stays put.test_badmac_migrates_pn_session_when_lid_shadow_exists— BadMac→migration→decrypt success.self_peer_target_prefers_lid_when_present/_falls_back_to_pn_without_lid/_errors_when_no_identity_known.peer_pkmsg_includes_meta_and_device_identity+peer_pkmsg_omits_device_identity_when_account_missing.wacore/libsignal/tests/session_divergence.rsmapping out what does and doesn't recover at the libsignal layer.8a54365f). Recovery within ~3s of first inbound msg from the previously stuck peer. 57 successful decrypts since, normal message flow restored (reactions + text, fanout including<device-identity>on every pkmsg).Test plan
Co-authorship
Codex helped pin the inbound
BadMac→try_pn_to_lid_migration_decryptextension (commit53c98e05) after I'd burned several rounds chasing the wrong layer.