fix!: migrate stale PN sessions to LID to fix NoSession decryption failures - #475
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplements PN→LID Signal session and identity migration on LID discovery, simplifies primary-phone login LID gating, prewarms LID↔PN mappings and retries decryption with on-the-fly migration for SessionNotFound, adjusts retry encryption-JID resolution, and adds end-to-end tests asserting LID-only session storage and migration behavior. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Client as Client
participant Msg as MessageHandler
participant Cache as SignalCache
participant DB as Persistence
participant Signal as SignalProtocol
Note over App,Client: LID discovery triggers migration
App->>Client: add_lid_pn_mapping(pn, lid)
Client->>Client: migrate_signal_sessions_on_lid_discovery(pn, lid)
loop device_id in 0..99
Client->>DB: read PN session (pn@*.device_id)
alt PN session exists
Client->>DB: check LID session (lid@*.device_id)
alt LID exists
Client->>DB: delete PN session
Client->>Cache: remove PN session from cache
else LID missing
Client->>DB: write session under LID key
Client->>DB: delete PN session
Client->>Cache: remove PN session from cache
end
Client->>DB: copy identity key if LID identity absent
end
end
Note over Msg,Signal: Decryption with migration retry
App->>Msg: incoming message
Msg->>Msg: cache_lid_pn_from_message(msg)
Msg->>Msg: resolve_encryption_jid(sender)
Msg->>Signal: attempt decrypt using resolved JID
alt Signal returns SessionNotFound
Msg->>Client: migrate_signal_sessions_on_lid_discovery(pn, lid)
Client->>DB: migrate sessions & identities
Msg->>Cache: reload sessions/identities from DB
Msg->>Signal: retry decrypt
alt retry succeeds
Msg->>App: deliver plaintext
else
Msg->>App: send retry-receipt fallback
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 41s —— View job Code Review: PN→LID Session Migration
Overall this is a well-structured fix for a real production bug. The approach of migrating sessions on-the-fly during decryption failure is sound and mirrors how WA Web handles the PN→LID transition. The E2E tests are thorough and the reproduction test ( Issues1. Nesting depth in SessionNotFound handler — The on-the-fly migration block is ~80 lines nested 8 levels deep inside the decryption error match arm. This makes it hard to follow the control flow (success → if let SignalProtocolError::SessionNotFound(_) = e {
if let Some(plaintext) = self.try_pn_lid_migration_decrypt(
&sender_encryption_jid, &signal_address, &parsed_message,
&mut adapter, &mut rng, info, &enc_type, padding_version,
).await {
any_success = true;
continue;
}
// fall through to retry receipt...
}2. Device range 0..=99 is iterated sequentially with two DB calls each —
3. The new logic populates 4. Missing session lock for migration in the The migration calls However, Nits5. 6. 7. What looks good
|
Bencher Report![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d47f978d5
ℹ️ 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_id_key = pn_jid.to_signal_address_string(); | ||
| let lid_id_key = lid_jid.to_signal_address_string(); |
There was a problem hiding this comment.
Identity records in this codebase are read/written with ProtocolAddress::as_str() (for example via Device::save_identity / load_identity), which includes the .0 suffix, but this migration builds identity keys with to_signal_address_string() (no suffix). As a result, PN identities are never found and migrated to the LID key actually used by Signal operations, so after PN→LID session migration the LID identity remains unset and trust checks silently fall back to TOFU for that contact/device.
Useful? React with 👍 / 👎.
| let signal_name = sender_encryption_jid.to_signal_address_string(); | ||
| if let Ok(Some(session_data)) = |
There was a problem hiding this comment.
This retry path reloads identity data with to_signal_address_string() (no .0), but identities are persisted under protocol-address keys (with .0). In the PN→LID migration flow that means load_identity will miss and the identity cache is not repopulated for the retried decrypt, which can leave the new LID address in an uninitialized trust state even though an identity exists in storage.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/sessions.rs (1)
146-155:⚠️ Potential issue | 🟠 MajorDon't send the "immediate" login repair through
ensure_e2e_sessions().
ensure_e2e_sessions()waits for offline delivery first on Lines 153-154, so this device-0 repair runs only after the window it is supposed to protect. And if that establish still fails, Lines 352-357 just log and returnOk(()), so the caller never learns that the primary-phone LID session is still missing. Callfetch_and_establish_sessions()directly here and propagate the error.⚙️ Minimal fix
- if let Err(e) = self - .ensure_e2e_sessions(std::slice::from_ref(&primary_phone_lid)) - .await - { - log::warn!("Failed to establish session with own device 0: {e}"); - } + self.fetch_and_establish_sessions(std::slice::from_ref(&primary_phone_lid)) + .await + .map(|_| ()) + .map_err(|e| { + anyhow::anyhow!( + "Failed to establish session with own device 0 {}: {}", + primary_phone_lid, + e + ) + })?;Also applies to: 347-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/sessions.rs` around lines 146 - 155, ensure_e2e_sessions currently waits for offline delivery (wait_for_offline_delivery_end) and therefore defers the "immediate" primary-phone login repair; instead, remove sending that immediate repair inside ensure_e2e_sessions and call fetch_and_establish_sessions(...) directly so the repair runs immediately and any error is propagated to the caller (do not swallow it). Also update the code path around where ensure_e2e_sessions previously logged and returned Ok(()) (the block handling the fetch/establish result) to propagate the error (return Err(...)) rather than only logging; use the existing function names fetch_and_establish_sessions and ensure_e2e_sessions to locate and modify these behaviors.src/retry.rs (1)
215-243:⚠️ Potential issue | 🟠 MajorUse the resolved address for the whole retry flow, not only
process_retry_key_bundle().This change writes and locks the session under
resolved_jid, but the registration-mismatch cleanup on Lines 215-243 and the DM base-key/session deletion on Lines 365-443 still useparticipant_jid.to_protocol_address(). For a PN retry receipt with a known LID mapping, that leaves the real@lidsession untouched and can keep the stale-session loop alive.Also applies to: 365-443, 551-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 215 - 243, The cleanup uses participant_jid.to_protocol_address() but the session was written/locked under the resolved JID; change all places in the retry flow that compute signal_address from participant_jid (including the registration-mismatch block around extract_registration_id_from_node(node), the DM base-key/session deletion code paths, and the other occurrences noted) to use resolved_jid.to_protocol_address() (the same resolved_jid used by process_retry_key_bundle()), so that calls to self.signal_cache.get_session(...), self.signal_cache.delete_session(...), and flush_signal_cache() operate on the resolved JID-backed session/address instead of the original participant_jid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/lid_pn.rs`:
- Around line 193-196: The identity migration currently only runs when
backend.get_session(&pn_session_key) returns Some(data) and the early continue
skips the identity-key migration; move the identity migration logic that updates
the contact trusted key (the “identity-key” block that migrates `@c.us` → `@lid`
TOFU) out of the session-present branch so it executes regardless of whether
backend.get_session(&pn_session_key) returns None; apply the same change to the
analogous block around the code mentioned (the second occurrence handling lines
~231-246) so identity migration survives PN session cleanup and subsequent
re-establishment.
- Around line 182-224: This migrator currently reads/writes only the persistence
backend (backend.get_session/put_session/delete_session) and then invalidates
only the PN in-memory cache (signal_cache.delete_session), which can race with
concurrent encrypt/decrypt and lose in-memory ratchet state; fix it by taking
the per-address session lock(s) from session_locks for the involved sender
addresses before reading/migrating (use the same lock key you use for
encrypt/decrypt, e.g. the pn_proto/lid_proto protocol address or their session
key), then inside that critical section: re-check/read the backend session, read
and migrate any in-memory Signal session state from signal_cache atomically
along with put_session/delete_session, and only release the lock after both
backend and cache have been updated; ensure you also use message_enqueue_locks
when migrating during per-chat processing if applicable and keep the existing
delete_session fallback behavior (references: backend.get_session,
backend.put_session, backend.delete_session, signal_cache.delete_session,
pn_proto, lid_proto, session_locks, message_enqueue_locks).
In `@src/message.rs`:
- Around line 913-955: The retry branch after calling message_decrypt needs to
special-case SignalProtocolError::DuplicatedMessage so post-migration duplicate
messages are silently ignored instead of being treated as SessionNotFound;
inside the Err(retry_err) arm of the retry_res match, match on retry_err and if
it is SignalProtocolError::DuplicatedMessage(..) simply log a debug/info message
and continue (do not set any_failure, do not fall through to the outer
SessionNotFound handling or trigger retry/undecryptable receipts), otherwise
keep the existing warning behavior and fallback handling; locate this change
around the message_decrypt retry block and the call to
self.clone().handle_decrypted_plaintext to implement the early-continue for
duplicated-message errors.
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 55-67: The test currently prints raw PII (pn_addr, lid_addr,
pn_user, lid_user) and full session lists (lid_sessions, pn_sessions) to logs
and assertion messages; change assertions and info! calls in the LID session
test to avoid outputting real JIDs by logging counts and masked identifiers
instead — for example, replace "{pn_user}@c.us" / "{lid_user}@lid" and
"{pn_sessions:?}" / "{lid_sessions:?}" with masked values (e.g.,
mask_jid(pn_addr), mask_jid(lid_addr)) or just the number of sessions
(pn_sessions.len(), lid_sessions.len()), update the assert! failure messages to
include context plus either counts or masked IDs, and modify the
info!("[{context}] LID-only sessions verified: {lid_sessions:?}") line to print
something like info!("[{context}] LID-only sessions verified: count={}
masked_first={}", lid_sessions.len(), mask_jid(lid_sessions.get(0))) using
existing helper or add a small mask_jid utility used across tests to redact real
JIDs; apply the same masking/count approach to the other mentioned occurrences
(uses of pn_addr, lid_addr, pn_sessions, lid_sessions at the other ranges).
- Around line 544-562: The test currently only calls
client_a.wait_for_text(test_text, 5) which can hide transient
UndecryptableMessage events; modify the test around the match on msg_result to
explicitly assert no UndecryptableMessage was emitted (use
client_a.assert_no_event or equivalent to check for UndecryptableMessage) before
treating the case as a success, and after the send verify the session mapping is
back under lid_addr (e.g., call the session lookup/assertion that checks the
session owner/address is lid_addr rather than pn_addr) so the regression is
caught deterministically.
---
Outside diff comments:
In `@src/client/sessions.rs`:
- Around line 146-155: ensure_e2e_sessions currently waits for offline delivery
(wait_for_offline_delivery_end) and therefore defers the "immediate"
primary-phone login repair; instead, remove sending that immediate repair inside
ensure_e2e_sessions and call fetch_and_establish_sessions(...) directly so the
repair runs immediately and any error is propagated to the caller (do not
swallow it). Also update the code path around where ensure_e2e_sessions
previously logged and returned Ok(()) (the block handling the fetch/establish
result) to propagate the error (return Err(...)) rather than only logging; use
the existing function names fetch_and_establish_sessions and ensure_e2e_sessions
to locate and modify these behaviors.
In `@src/retry.rs`:
- Around line 215-243: The cleanup uses participant_jid.to_protocol_address()
but the session was written/locked under the resolved JID; change all places in
the retry flow that compute signal_address from participant_jid (including the
registration-mismatch block around extract_registration_id_from_node(node), the
DM base-key/session deletion code paths, and the other occurrences noted) to use
resolved_jid.to_protocol_address() (the same resolved_jid used by
process_retry_key_bundle()), so that calls to
self.signal_cache.get_session(...), self.signal_cache.delete_session(...), and
flush_signal_cache() operate on the resolved JID-backed session/address instead
of the original participant_jid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9e25ed7-c1df-4ce0-a3ce-0bf75d4dc51a
📒 Files selected for processing (6)
src/client/lid_pn.rssrc/client/sessions.rssrc/message.rssrc/retry.rstests/e2e/tests/lid_sessions.rswacore/src/messages.rs
| let backend = self.persistence_manager.backend(); | ||
|
|
||
| for device_id in 0..=99u16 { | ||
| let pn_jid = Jid::pn_device(pn.to_string(), device_id); | ||
| let lid_jid = Jid::lid_device(lid.to_string(), device_id); | ||
|
|
||
| let pn_proto = pn_jid.to_protocol_address(); | ||
| let lid_proto = lid_jid.to_protocol_address(); | ||
| let pn_session_key = pn_proto.to_string(); | ||
| let lid_session_key = lid_proto.to_string(); | ||
|
|
||
| // Check for PN session | ||
| let session_data = match backend.get_session(&pn_session_key).await { | ||
| Ok(Some(data)) => data, | ||
| Ok(None) => continue, | ||
| Err(e) => { | ||
| warn!("Failed to read PN session {pn_session_key}: {e}"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| match backend.get_session(&lid_session_key).await { | ||
| Ok(Some(_)) => { | ||
| // LID session already exists — just clean up the stale PN session | ||
| if let Err(e) = backend.delete_session(&pn_session_key).await { | ||
| warn!("Failed to delete stale PN session {pn_session_key}: {e}"); | ||
| } | ||
| self.signal_cache.delete_session(&pn_proto).await; | ||
| info!( | ||
| "Deleted stale PN session {pn_session_key} (LID {lid_session_key} exists)" | ||
| ); | ||
| } | ||
| Ok(None) => { | ||
| // No LID session — migrate | ||
| if let Err(e) = backend.put_session(&lid_session_key, &session_data).await { | ||
| warn!("Failed to write LID session {lid_session_key}: {e}"); | ||
| continue; | ||
| } | ||
| if let Err(e) = backend.delete_session(&pn_session_key).await { | ||
| warn!("Failed to delete PN session {pn_session_key} after migration: {e}"); | ||
| } | ||
| self.signal_cache.delete_session(&pn_proto).await; | ||
| info!("Migrated session {pn_session_key} -> {lid_session_key}"); |
There was a problem hiding this comment.
Don't migrate Signal state behind the cache and without per-address locks.
wacore/src/store/signal_cache.rs:28-65 shows session state can exist as dirty or negative cache entries in memory. This helper reads and writes only through backend.get_session()/put_session() and then invalidates only the PN cache key, so a concurrent encrypt/decrypt can race the migration, an unflushed PN ratchet update can be lost, and a cached LID miss can survive until some later reload. Please take the relevant session_locks and migrate the cache state atomically with the persistent rows. As per coding guidelines: Use session_locks to serialize per-sender Signal encrypt/decrypt operations and message_enqueue_locks to serialize per-chat incoming message processing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/lid_pn.rs` around lines 182 - 224, This migrator currently
reads/writes only the persistence backend
(backend.get_session/put_session/delete_session) and then invalidates only the
PN in-memory cache (signal_cache.delete_session), which can race with concurrent
encrypt/decrypt and lose in-memory ratchet state; fix it by taking the
per-address session lock(s) from session_locks for the involved sender addresses
before reading/migrating (use the same lock key you use for encrypt/decrypt,
e.g. the pn_proto/lid_proto protocol address or their session key), then inside
that critical section: re-check/read the backend session, read and migrate any
in-memory Signal session state from signal_cache atomically along with
put_session/delete_session, and only release the lock after both backend and
cache have been updated; ensure you also use message_enqueue_locks when
migrating during per-chat processing if applicable and keep the existing
delete_session fallback behavior (references: backend.get_session,
backend.put_session, backend.delete_session, signal_cache.delete_session,
pn_proto, lid_proto, session_locks, message_enqueue_locks).
…ilures Messages from the primary phone arrive from a LID address but the session was stored under PN from an old pairing, causing perpetual SessionNotFound errors. This aligns with WA Web's createSignalAddress() which always resolves PN→LID before any session operation. Changes: - Add migrate_signal_sessions_on_lid_discovery() to move PN sessions and identity keys to LID when a new LID-PN mapping is discovered - At login, proactively check own device 0 session and migrate PN→LID or establish fresh via fetch_and_establish_sessions (bypasses offline wait) - On SessionNotFound for LID address, attempt PN→LID migration and retry decryption before falling back to retry receipt; handle DuplicatedMessage gracefully in the post-migration retry path - Use resolve_encryption_jid() in retry receipt handling to ensure all session operations use the resolved LID address - Deduplicate sender_encryption_jid resolution into cache_lid_pn_from_message() + resolve_encryption_jid(); extract try_pn_to_lid_migration_decrypt() helper to reduce nesting - Populate sender_alt for self-messages so LID-PN cache warms from own device messages Identity migration runs independently of session migration (survives session deletion and re-establishment). Identity keys use protocol address format (with .0 suffix) matching the Signal store backend. BREAKING: establish_primary_phone_session_immediate() now actively migrates and establishes sessions instead of passively checking.
1d47f97 to
a9b3c06
Compare
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 6s —— View job Code Review: PN→LID Session Migration (post-fix commit a9b3c06)
This is a solid fix for a real production bug. The commit addresses most of the review feedback from CodeRabbit and the previous Claude review (extracted One real issue and a few observations below. Bug:
|
- establish_primary_phone_session_immediate now only logs session state; migration is fully lazy via try_pn_to_lid_migration_decrypt on first message - Fix is_peer check in retry handler: after PN→LID resolution the user part is LID, so also compare against device_snapshot.lid (not just pn) - Test: mask JIDs in logs, assert no UndecryptableMessage + verify session under LID after migration, bump timeout to 15s for CI stability
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b3c06af8
ℹ️ 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 let Ok(Some(session_data)) = backend.get_session(pn_addr_key).await { | ||
| match backend.get_session(lid_addr_key).await { | ||
| Ok(Some(_)) => { | ||
| if let Err(e) = backend.delete_session(pn_addr_key).await { | ||
| warn!("Failed to delete stale PN session {pn_addr_key}: {e}"); |
There was a problem hiding this comment.
This migration path reads/writes sessions directly through backend and then calls signal_cache.delete_session without acquiring the per-address session_lock_for lock that the send/decrypt/retry paths rely on. If a message is concurrently advancing the PN session in cache, migration can copy an older backend record to LID and then drop the newer PN state before it is flushed, which can roll back ratchet state and trigger follow-up BadMac/SessionNotFound failures for active chats.
Useful? React with 👍 / 👎.
| // Device existence check (matches WhatsApp Web's WAWebApiDeviceList.hasDevice). | ||
| // This prevents processing retry receipts from unknown/stale devices. | ||
| // Resolve PN→LID so all session operations use the correct address | ||
| let participant_jid = self.resolve_encryption_jid(&participant_jid).await; |
There was a problem hiding this comment.
Normalizing participant_jid to LID here happens before peer detection, but is_peer still compares only against device_snapshot.pn.user; when a retry receipt from our own account arrives as PN and resolves to LID, is_peer becomes false. That bypasses the stricter peer-device registration-ID change rejection in process_retry_key_bundle, so a peer reinstall/key change can be treated as a normal contact session replacement instead of being blocked.
Useful? React with 👍 / 👎.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 34s —— View job Code Review: PN→LID Session Migration (commits a9b3c06, 9956dc6)
Well-structured fix for a real production bug. The approach — on-the-fly PN→LID migration during Previous reviews have already covered several items that were fixed in commit 9956dc6. I'll focus on what remains and what's new. Verified as fixed (from earlier reviews)
Remaining observations1. Session migration still bypasses The migration reads/writes through
However, if two concurrent messages trigger migration for different LID addresses that map to the same PN (theoretically impossible in WA's addressing model, but worth noting), there's a potential race. Since this is defense-in-depth rather than a practical bug, I'd consider it a low-priority improvement. 2. Login session check is lazy — good tradeoff — The previous 3. The identity is loaded from backend using 4. The extraction from the previous inline block eliminates ~75 lines of duplication. The LID-PN pair extraction handles both directions (LID sender with PN alt, PN sender with LID alt) and includes the self-message case via 5. E2E test
info!(
"LID session count after 5 sends: {} ({lid_sessions:?})",
lid_sessions.len()
);Minor nit — could mask these too for consistency. 6. The The resolve happens early, which is good — all downstream operations (registration ID check at line 221, SummaryThe PR is solid. The core fix (on-the-fly migration + lazy login check) is correct and well-tested. The only substantive item from previous reviews that remains is the session lock concern in |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/e2e/tests/lid_sessions.rs (1)
195-198:⚠️ Potential issue | 🟠 MajorMask the remaining raw addresses in logs and assertion messages.
mask_addr()is used in the helper, but these paths still print live@c.us/@lidaddresses into CI output and failure text. Please reusemask_addr()here too, or replace the identifiers with counts/context only.Based on learnings: Applies to **/test.rs : Never use real PII (phone numbers and JIDs) in test code; use fictitious values instead.
Also applies to: 248-248, 280-283, 407-423, 530-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 195 - 198, The logs and assertion messages print real JIDs (e.g., "@c.us" / "@lid") from lid_sessions which is PII; update the logging and assertions to call the existing mask_addr() helper for each JID (or replace with non-identifying summaries like counts) wherever lid_sessions or similar session JIDs are used (e.g., the info! call referencing lid_sessions.len() and entries, and the other occurrences noted at lines ~248, ~280-283, ~407-423, ~530-538); locate usages of lid_sessions, any debug/info! or assert messages that interpolate JIDs and wrap those identifiers with mask_addr(jid) or use a masked/map view so no raw addresses appear in test output.src/client/lid_pn.rs (1)
189-206:⚠️ Potential issue | 🔴 CriticalMake the PN→LID move atomic with
signal_cacheand the session locks.This migrator reads/writes only through the backend and then invalidates only the PN cache key. If the PN session is dirty in
signal_cache, or the LID side is cached as a miss, you can lose newer ratchet state and still fail the immediate LID re-check even though SQLite was updated. Move the session under the relevantsession_locksand update/invalidate both cache entries in the same critical section as the persistence writes.As per coding guidelines: Use
session_locksto serialize per-sender Signal encrypt/decrypt operations andmessage_enqueue_locksto serialize per-chat incoming message processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/lid_pn.rs` around lines 189 - 206, The PN→LID migration must be done under the per-sender session_locks and update/invalidate both cache entries atomically: acquire the session_locks for the sender (using the same key you use for pn_proto/pn_addr_key), then re-check backend.get_session(pn_addr_key) and backend.get_session(lid_addr_key) while holding the lock, perform backend.put_session(lid_addr_key, &session_data) and backend.delete_session(pn_addr_key) inside that locked section, and update the signal_cache for both sides inside the same critical section (write/insert or invalidate the LID cache entry and then delete/invalidate the PN cache entry) before releasing the lock so no concurrent Signal operations can see mixed state; use the existing symbols backend.get_session, backend.put_session, backend.delete_session, self.signal_cache (delete/update), session_locks and pn_proto/pn_addr_key/lid_addr_key to locate and implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/sessions.rs`:
- Around line 272-275: The check in the block that inspects device_snapshot.lid
(the let Some(ref own_lid) = device_snapshot.lid else { ... return Ok(()); })
must not silently no-op; instead, when get_device_snapshot() yields lid == None,
enqueue or re-schedule the primary-phone migration/prekey fetch work to run once
the own LID becomes available (e.g., push a task to your existing retry/worker
queue or register a callback/listener that triggers the same primary-phone
session check when the appstate/offline sync populates lid). Update the logic
around get_device_snapshot(), own_lid, and the primary phone session check so
that the function returns a pending/rescheduled outcome rather than Ok(()) when
lid is missing, and ensure the re-scheduled action uses the same code path that
runs when own_lid is present.
In `@src/message.rs`:
- Around line 872-889: The current try_pn_to_lid_migration_decrypt call (used in
the message handling path) returns only a bool so all post-migration failures
collapse into RetryReason::NoSession; change try_pn_to_lid_migration_decrypt to
return a typed outcome (e.g., Result<(), SignalProtocolError> or a small enum
like MigrationOutcome { Success, DuplicateMessage, Err(SignalProtocolError) })
and propagate the specific error back to the caller so the caller can
distinguish DuplicatedMessage, UntrustedIdentity, BadMac/InvalidMessage,
InvalidPreKeyId, etc.; update the call sites in the message processing flow (the
block around try_pn_to_lid_migration_decrypt and the similar block at 1172-1259)
to match on the new outcome and continue normal remediation paths instead of
always treating false as NoSession.
In `@src/retry.rs`:
- Around line 178-179: In handle_retry_receipt(), do not rebind participant_jid
with the resolved value; call resolve_encryption_jid(&participant_jid).await and
store the result in a new variable (e.g., encryption_jid or resolved_jid) so the
original raw participant_jid remains available for the PN peer check and group
resend logic; update calls that need the session address (such as
prepare_group_retry_stanza and any session operations) to use the new
encryption_jid, leaving the original participant_jid unchanged for peer
comparisons and other logic.
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 34-50: The helper scan_sessions currently checks only the
persistent backend via persistence_manager().backend() / backend.get_session
which misses in-memory cached sessions; update the test to also assert the
live/runtime Signal store by querying Client.signal_cache (or the in-memory
runtime store API) for each address (e.g., inside scan_sessions or the callers
around lines 54-87) or, alternatively, force a client reconnect/evict cache
(call the client's reconnect or cache-clear method) before each backend
assertion so the test exercises cache/backend consistency and fails when the
runtime store differs from the SQLite backend.
---
Duplicate comments:
In `@src/client/lid_pn.rs`:
- Around line 189-206: The PN→LID migration must be done under the per-sender
session_locks and update/invalidate both cache entries atomically: acquire the
session_locks for the sender (using the same key you use for
pn_proto/pn_addr_key), then re-check backend.get_session(pn_addr_key) and
backend.get_session(lid_addr_key) while holding the lock, perform
backend.put_session(lid_addr_key, &session_data) and
backend.delete_session(pn_addr_key) inside that locked section, and update the
signal_cache for both sides inside the same critical section (write/insert or
invalidate the LID cache entry and then delete/invalidate the PN cache entry)
before releasing the lock so no concurrent Signal operations can see mixed
state; use the existing symbols backend.get_session, backend.put_session,
backend.delete_session, self.signal_cache (delete/update), session_locks and
pn_proto/pn_addr_key/lid_addr_key to locate and implement this change.
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 195-198: The logs and assertion messages print real JIDs (e.g.,
"@c.us" / "@lid") from lid_sessions which is PII; update the logging and
assertions to call the existing mask_addr() helper for each JID (or replace with
non-identifying summaries like counts) wherever lid_sessions or similar session
JIDs are used (e.g., the info! call referencing lid_sessions.len() and entries,
and the other occurrences noted at lines ~248, ~280-283, ~407-423, ~530-538);
locate usages of lid_sessions, any debug/info! or assert messages that
interpolate JIDs and wrap those identifiers with mask_addr(jid) or use a
masked/map view so no raw addresses appear in test output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65090fc5-e6dc-436e-9ffa-cfe94ce7f240
📒 Files selected for processing (6)
src/client/lid_pn.rssrc/client/sessions.rssrc/message.rssrc/retry.rstests/e2e/tests/lid_sessions.rswacore/src/messages.rs
| let Some(ref own_lid) = device_snapshot.lid else { | ||
| log::debug!("No own LID yet, skipping primary phone session check"); | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
Don't silently no-op when the own LID hasn't been populated yet.
get_device_snapshot() can legitimately return lid = None during login before appstate/offline sync fills it in. Returning Ok(()) here means the proactive own-device migration/prekey fetch never runs on that path, so device 0 can remain PN-only until some later flow repairs it. Please defer or reschedule this step once the own LID is available instead of skipping it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/sessions.rs` around lines 272 - 275, The check in the block that
inspects device_snapshot.lid (the let Some(ref own_lid) = device_snapshot.lid
else { ... return Ok(()); }) must not silently no-op; instead, when
get_device_snapshot() yields lid == None, enqueue or re-schedule the
primary-phone migration/prekey fetch work to run once the own LID becomes
available (e.g., push a task to your existing retry/worker queue or register a
callback/listener that triggers the same primary-phone session check when the
appstate/offline sync populates lid). Update the logic around
get_device_snapshot(), own_lid, and the primary phone session check so that the
function returns a pending/rescheduled outcome rather than Ok(()) when lid is
missing, and ensure the re-scheduled action uses the same code path that runs
when own_lid is present.
| // Try PN→LID session migration before sending retry receipt | ||
| if let SignalProtocolError::SessionNotFound(_) = e { | ||
| if self | ||
| .try_pn_to_lid_migration_decrypt( | ||
| sender_encryption_jid, | ||
| &signal_address, | ||
| &parsed_message, | ||
| &mut adapter, | ||
| &mut rng, | ||
| &enc_type, | ||
| padding_version, | ||
| info, | ||
| ) | ||
| .await | ||
| { | ||
| any_success = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Don’t collapse post-migration failures into NoSession.
try_pn_to_lid_migration_decrypt() returns false for every retry error other than DuplicatedMessage, so the caller on Line 895 always falls back to RetryReason::NoSession. That skips the existing UntrustedIdentity, BadMac/InvalidMessage, and InvalidPreKeyId remediation paths once a PN session has been migrated to LID, and can leave the stale LID session/identity state in place. Return the retry error, or a typed outcome enum, so the normal handlers can continue after migration.
Also applies to: 1172-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 872 - 889, The current
try_pn_to_lid_migration_decrypt call (used in the message handling path) returns
only a bool so all post-migration failures collapse into RetryReason::NoSession;
change try_pn_to_lid_migration_decrypt to return a typed outcome (e.g.,
Result<(), SignalProtocolError> or a small enum like MigrationOutcome { Success,
DuplicateMessage, Err(SignalProtocolError) }) and propagate the specific error
back to the caller so the caller can distinguish DuplicatedMessage,
UntrustedIdentity, BadMac/InvalidMessage, InvalidPreKeyId, etc.; update the call
sites in the message processing flow (the block around
try_pn_to_lid_migration_decrypt and the similar block at 1172-1259) to match on
the new outcome and continue normal remediation paths instead of always treating
false as NoSession.
| async fn scan_sessions( | ||
| backend: &dyn SignalStore, | ||
| user: &str, | ||
| server: &str, | ||
| ) -> anyhow::Result<Vec<String>> { | ||
| let mut results = Vec::new(); | ||
| for device_id in 0..=99u16 { | ||
| let addr = if device_id == 0 { | ||
| format!("{user}@{server}.0") | ||
| } else { | ||
| format!("{user}:{device_id}@{server}.0") | ||
| }; | ||
| if backend.get_session(&addr).await?.is_some() { | ||
| results.push(addr); | ||
| } | ||
| } | ||
| Ok(results) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
These helpers only assert SQLite state, not the live Signal store.
persistence_manager().backend() bypasses Client.signal_cache, so a PN session or cached LID miss left in memory would still satisfy these checks until a reconnect. For this regression, add a live-session assertion via the runtime store, or force a reconnect before each storage assertion, so the suite exercises cache/backend consistency too.
Also applies to: 54-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/lid_sessions.rs` around lines 34 - 50, The helper
scan_sessions currently checks only the persistent backend via
persistence_manager().backend() / backend.get_session which misses in-memory
cached sessions; update the test to also assert the live/runtime Signal store by
querying Client.signal_cache (or the in-memory runtime store API) for each
address (e.g., inside scan_sessions or the callers around lines 54-87) or,
alternatively, force a client reconnect/evict cache (call the client's reconnect
or cache-clear method) before each backend assertion so the test exercises
cache/backend consistency and fails when the runtime store differs from the
SQLite backend.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9956dc65ef
ℹ️ 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".
| // Device existence check (matches WhatsApp Web's WAWebApiDeviceList.hasDevice). | ||
| // This prevents processing retry receipts from unknown/stale devices. | ||
| // Resolve PN→LID so all session operations use the correct address | ||
| let participant_jid = self.resolve_encryption_jid(&participant_jid).await; |
There was a problem hiding this comment.
Normalizing participant_jid to LID here rewrites the value that is later used as the outgoing participant attribute in prepare_group_retry_stanza, even when the group is still PN-addressed (addressing_mode="pn"). In that case we can emit a retry stanza with PN addressing mode but a LID participant, which can target the wrong namespace and cause retries to be ignored for affected groups/devices. Keep the original participant JID for stanza addressing and only use the resolved JID for Signal session/identity lookups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/retry.rs (2)
178-179:⚠️ Potential issue | 🟠 MajorRebinding
participant_jidloses the original address for downstream use.Reassigning
participant_jidat line 179 means the original PN address is no longer available. While theis_peercheck (lines 193-200) now correctly handles both PN and LID comparisons, this creates two issues:
- Line 470 calls
resolve_encryption_jid(&participant_jid)again on the already-resolved JID (redundant/no-op).- Line 484 passes the resolved
participant_jidtoprepare_group_retry_stanza, but the original participant address may be needed for proper envelope addressing.Consider keeping the original and using a separate variable for the resolved address:
Suggested fix
- // Resolve PN→LID so all session operations use the correct address - let participant_jid = self.resolve_encryption_jid(&participant_jid).await; + // Resolve PN→LID so session operations use the correct address + let resolved_participant_jid = self.resolve_encryption_jid(&participant_jid).await; - let sender_device_id = participant_jid.device() as u32; - let sender_user = participant_jid.user.clone(); + let sender_device_id = resolved_participant_jid.device() as u32; + let sender_user = resolved_participant_jid.user.clone(); if !self.has_device(&sender_user, sender_device_id).await { // ... } // Check if this is a retry from our own device (peer). let device_snapshot = self.persistence_manager.get_device_snapshot().await; let is_peer = device_snapshot .pn .as_ref() - .is_some_and(|our_pn| participant_jid.user == our_pn.user) + .is_some_and(|our_pn| participant_jid.is_same_user_as(our_pn)) || device_snapshot .lid .as_ref() - .is_some_and(|our_lid| participant_jid.user == our_lid.user); + .is_some_and(|our_lid| participant_jid.is_same_user_as(our_lid)); // ... if !receipt.source.chat.is_status_broadcast() { let key_bundle_result = self - .process_retry_key_bundle(node, &participant_jid, is_peer) + .process_retry_key_bundle(node, &resolved_participant_jid, is_peer) .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 178 - 179, Rebinding participant_jid loses the original PN address and causes redundant/resolved use later; keep the original PN JID and store the resolved LID in a new variable (e.g., resolved_participant_jid) by calling resolve_encryption_jid(&participant_jid). Update downstream uses: use resolved_participant_jid for session operations and is_peer comparisons when LID is required, avoid calling resolve_encryption_jid again at line 470, and pass the original participant_jid into prepare_group_retry_stanza when the original envelope addressing is needed.
193-200: 🧹 Nitpick | 🔵 Trivial
is_peerexpansion correctly handles both PN and LID addresses.The dual check against
our_pn.userandour_lid.userensures peer detection works regardless of whether the participant JID came in as PN or LID format. This aligns with the PR's goal of supporting LID-first addressing.However, consider using
is_same_user_as()instead of direct user string comparison for more robust JID equivalence checking:let is_peer = device_snapshot .pn .as_ref() - .is_some_and(|our_pn| participant_jid.user == our_pn.user) + .is_some_and(|our_pn| participant_jid.is_same_user_as(our_pn)) || device_snapshot .lid .as_ref() - .is_some_and(|our_lid| participant_jid.user == our_lid.user); + .is_some_and(|our_lid| participant_jid.is_same_user_as(our_lid));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 193 - 200, The is_peer check currently compares participant_jid.user to our_pn.user and our_lid.user directly; replace those direct string comparisons in the is_peer expression (referencing device_snapshot.pn, device_snapshot.lid and participant_jid.user / our_pn.user / our_lid.user) with the more robust JID equality helper is_same_user_as(), i.e., call is_same_user_as(participant_jid, our_pn) and is_same_user_as(participant_jid, our_lid) (or equivalent) inside the is_some_and closures so peer detection uses the canonical JID equivalence function.src/client/sessions.rs (1)
262-275: 🧹 Nitpick | 🔵 TrivialFunction name no longer reflects behavior; early return when LID is missing may hide incomplete state.
The function
establish_primary_phone_session_immediatenow only logs session state without establishing anything. The name is misleading—consider renaming tolog_primary_phone_session_stateor similar to match the doc comment.Additionally, returning
Ok(())whendevice_snapshot.lidisNone(lines 272-274) means the caller (insrc/client.rsaround line 1920) will proceed without knowing the proactive check was skipped. Since the caller only logs a warning onErr, it won't be aware that LID was missing at login time. If this is intentional (lazy migration on first message), consider returning a sentinel or logging atwarn!level to improve observability.♻️ Suggested improvements
- /// Log primary phone (device 0) session state at login. - /// Migration is lazy via try_pn_to_lid_migration_decrypt on first message. - pub(crate) async fn establish_primary_phone_session_immediate(&self) -> Result<()> { + /// Log primary phone (device 0) session state at login. + /// Migration is lazy via try_pn_to_lid_migration_decrypt on first message. + pub(crate) async fn log_primary_phone_session_state(&self) -> Result<()> { let device_snapshot = self.persistence_manager.get_device_snapshot().await; let own_pn = device_snapshot .pn .clone() .ok_or_else(|| anyhow::Error::from(crate::client::ClientError::NotLoggedIn))?; let Some(ref own_lid) = device_snapshot.lid else { - log::debug!("No own LID yet, skipping primary phone session check"); + log::warn!("No own LID yet, skipping primary phone session check (will migrate on first message)"); return Ok(()); };tests/e2e/tests/lid_sessions.rs (3)
194-198:⚠️ Potential issue | 🟡 MinorRaw session addresses logged in debug output.
Line 196 logs
{lid_sessions:?}which contains full JID-based session addresses. This can expose real user identifiers in CI logs.Suggested fix
let lid_sessions = scan_sessions(&*backend_a, &lid_b.user, "lid").await?; info!( - "LID session count after 5 sends: {} ({lid_sessions:?})", - lid_sessions.len() + "LID session count after 5 sends: {}, first: {}", + lid_sessions.len(), + lid_sessions.first().map(|s| mask_addr(s)).unwrap_or_default() );Based on learnings: Applies to **/test.rs : Never use real PII (phone numbers and JIDs) in test code; use fictitious values instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 194 - 198, The test currently logs raw session addresses via the info! call that prints `{lid_sessions:?}`, exposing JID/PII; update the logging in tests/e2e/tests/lid_sessions.rs to avoid printing real JIDs by either logging only lid_sessions.len() or mapping/masking each entry returned by scan_sessions(&*backend_a, &lid_b.user, "lid") into a non-PII form (e.g., replace domain/localpart with a fixed placeholder or generate sequential fake IDs) before passing to info!; ensure the change touches the lid_sessions variable usage and the info! call so no real phone numbers/JIDs appear in CI logs.
19-31: 🧹 Nitpick | 🔵 TrivialMasking helper partially redacts addresses; sufficient for test logs but not fully anonymized.
The
mask_addrfunction reveals the first 2 and last 2 characters of the user portion (e.g.,55...99@lid.0). While this reduces exposure, patterns may still be identifiable in production logs. Since this is e2e test code that connects to live accounts, consider logging only counts or using fully synthetic placeholders in CI output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 19 - 31, The mask_addr helper leaks fragments of the user part (mask_addr) which may expose patterns; update it to fully anonymize user identifiers in test logs by replacing the entire local part with a fixed placeholder or a deterministic safe token (e.g., "<REDACTED>" or "user-<short-hash>") while preserving the domain for test diagnostics, and ensure the function returns "<REDACTED>@domain" (or similar) for inputs containing '@' and leaves non-address strings unchanged.
246-248:⚠️ Potential issue | 🟡 MinorRaw PN address logged.
Line 248 logs the full
pn_addrwhich contains the real user identifier.- info!("Injected PN session at {pn_addr}"); + info!("Injected PN session at {}", mask_addr(&pn_addr));Based on learnings: Applies to **/test.rs : Never use real PII (phone numbers and JIDs) in test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 246 - 248, The test currently logs a raw PII-containing value via pn_addr (the string like "{}@c.us.0") after calling backend_a.put_session(&pn_addr, &lid_session_data); change the logging to avoid printing the real JID/phone: mask or redact pn_addr (e.g., replace the localpart with a fixed token, hash it, or log only the domain), or log a non-PII placeholder (e.g., "Injected PN session for <redacted>") instead; update the info! call that references pn_addr so it never emits the full pn_addr value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 528-536: The test is logging full PII-like addresses via the info!
calls using lid_addr and pn_addr; change the logging to redact or obfuscate
those values (e.g., log a masked form, last N chars, or a short hash) instead of
the raw lid_addr/pn_addr, and ensure any similar info! usage in this file or
other tests (e.g., the "Moved session" message and the earlier "Read LID
session" message) uses the same masked representation; keep
backend_a.put_session and backend_a.delete_session unchanged, only adjust what
is passed into info! to avoid printing raw addresses.
- Around line 405-421: Tests are currently logging raw PII via lid_addr and
pn_addr; update the assertions and info! calls to avoid printing real addresses
by replacing lid_addr and pn_addr with a redacted or masked representation
(e.g., mask_local_part or a constant like "<REDACTED_DEVICE>") before use;
change the assert! messages and info! invocations that reference lid_addr and
pn_addr (the variables lid_addr and pn_addr and the info!/assert! sites in the
own-device test) to use the redacted string or a deterministic hash so no raw
PII appears in test output.
---
Duplicate comments:
In `@src/retry.rs`:
- Around line 178-179: Rebinding participant_jid loses the original PN address
and causes redundant/resolved use later; keep the original PN JID and store the
resolved LID in a new variable (e.g., resolved_participant_jid) by calling
resolve_encryption_jid(&participant_jid). Update downstream uses: use
resolved_participant_jid for session operations and is_peer comparisons when LID
is required, avoid calling resolve_encryption_jid again at line 470, and pass
the original participant_jid into prepare_group_retry_stanza when the original
envelope addressing is needed.
- Around line 193-200: The is_peer check currently compares participant_jid.user
to our_pn.user and our_lid.user directly; replace those direct string
comparisons in the is_peer expression (referencing device_snapshot.pn,
device_snapshot.lid and participant_jid.user / our_pn.user / our_lid.user) with
the more robust JID equality helper is_same_user_as(), i.e., call
is_same_user_as(participant_jid, our_pn) and is_same_user_as(participant_jid,
our_lid) (or equivalent) inside the is_some_and closures so peer detection uses
the canonical JID equivalence function.
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 194-198: The test currently logs raw session addresses via the
info! call that prints `{lid_sessions:?}`, exposing JID/PII; update the logging
in tests/e2e/tests/lid_sessions.rs to avoid printing real JIDs by either logging
only lid_sessions.len() or mapping/masking each entry returned by
scan_sessions(&*backend_a, &lid_b.user, "lid") into a non-PII form (e.g.,
replace domain/localpart with a fixed placeholder or generate sequential fake
IDs) before passing to info!; ensure the change touches the lid_sessions
variable usage and the info! call so no real phone numbers/JIDs appear in CI
logs.
- Around line 19-31: The mask_addr helper leaks fragments of the user part
(mask_addr) which may expose patterns; update it to fully anonymize user
identifiers in test logs by replacing the entire local part with a fixed
placeholder or a deterministic safe token (e.g., "<REDACTED>" or
"user-<short-hash>") while preserving the domain for test diagnostics, and
ensure the function returns "<REDACTED>@domain" (or similar) for inputs
containing '@' and leaves non-address strings unchanged.
- Around line 246-248: The test currently logs a raw PII-containing value via
pn_addr (the string like "{}@c.us.0") after calling
backend_a.put_session(&pn_addr, &lid_session_data); change the logging to avoid
printing the real JID/phone: mask or redact pn_addr (e.g., replace the localpart
with a fixed token, hash it, or log only the domain), or log a non-PII
placeholder (e.g., "Injected PN session for <redacted>") instead; update the
info! call that references pn_addr so it never emits the full pn_addr value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dd73ffa-31df-4bd9-bcc5-699544017b57
📒 Files selected for processing (3)
src/client/sessions.rssrc/retry.rstests/e2e/tests/lid_sessions.rs
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)
178-189:⚠️ Potential issue | 🟠 MajorDon't reject valid retries on a namespace-only device miss.
resolved_jidfixes the session path, but the guard on Lines 181-188 still callshas_device()withparticipant_jid.useronly.src/client/device_registry.rs:99-135caches device rows under the backend's actual namespace, so after a reconnect/coldlid_pn_cachea PN retry can miss a LID-backed record here and return on Line 188 beforeprocess_retry_key_bundle()or resend runs. Probe both the raw and resolved users before dropping the receipt.Suggested change
- let sender_device_id = participant_jid.device() as u32; - let sender_user = participant_jid.user.clone(); - if !self.has_device(&sender_user, sender_device_id).await { + let sender_device_id = participant_jid.device() as u32; + let sender_user = participant_jid.user.clone(); + let resolved_sender_user = resolved_jid.user.clone(); + let has_sender_device = self.has_device(&sender_user, sender_device_id).await + || (resolved_sender_user != sender_user + && self.has_device(&resolved_sender_user, sender_device_id).await); + if !has_sender_device { warn!( - "handle_retry_receipt: device not found for device={}, user={}", - sender_device_id, sender_user + "handle_retry_receipt: device not found for device={}, user={} (resolved={})", + sender_device_id, sender_user, resolved_sender_user ); return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 178 - 189, The guard that bails out when a device is missing uses has_device(&sender_user, sender_device_id) but only checks the original participant_jid.user, which can falsely miss a LID-backed cached record that was stored under the backend namespace resolved by resolve_encryption_jid; update the check in the retry handling (around resolved_jid, participant_jid, sender_device_id) to probe both the raw user and the resolved user (i.e., call has_device with participant_jid.user and also with resolved_jid.user) and only return early if both checks fail so process_retry_key_bundle() / resend logic can still run for namespace-mapped devices.
♻️ Duplicate comments (2)
tests/e2e/tests/lid_sessions.rs (2)
195-198:⚠️ Potential issue | 🟠 MajorFinish redacting live session addresses in test output.
These messages still emit raw identifiers via
{lid_sessions:?},{lid_addr}, and{pn_addr}. A failing run will leak real test-account JIDs into CI logs even thoughmask_addr()is already available in this file. Use masked values or counts consistently here too.Based on learnings: Applies to **/test.rs : Never use real PII (phone numbers and JIDs) in test code; use fictitious values instead.
Also applies to: 280-283, 407-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 195 - 198, The info logs currently print real identifiers (lid_sessions via "{lid_sessions:?}", and variables lid_addr and pn_addr) which leaks PII; update every log site (e.g., the info! call referencing lid_sessions, and the places around lid_addr and pn_addr noted) to either log only counts (lid_sessions.len()) or map each address through the existing mask_addr() helper before formatting (e.g., lid_sessions.iter().map(|a| mask_addr(a)).collect::<Vec<_>>() or mask_addr(&lid_addr), mask_addr(&pn_addr)); ensure mask_addr is in scope and replace any "{...:?}" usages that would emit raw JIDs/phone numbers with the masked values or counts consistently across the file (also at the other ranges referenced).
33-50: 🧹 Nitpick | 🔵 TrivialThese invariants still bypass the live Signal store.
scan_sessions()only checkspersistence_manager().backend(), so every caller can pass whileClient.signal_cachestill holds a stale PN session or a cached LID miss. Add a live-store assertion here, or force a reconnect/cache clear before each invariant check, so the suite catches cache/backend divergence too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/lid_sessions.rs` around lines 33 - 50, scan_sessions currently only queries the persistence_manager().backend() via backend.get_session, which lets callers pass while Client.signal_cache remains stale; update scan_sessions to either force a live-store check or clear/reconnect the client cache before the loop: add an argument to accept the test Client (or a cache-control helper) and call the client's cache clear or reconnect method (eg. Client.clear_signal_cache() / Client.reconnect()) or, for each addr, also query the live store via the client's live-session lookup and assert it matches backend.get_session(addr); ensure you reference scan_sessions, backend.get_session, and Client.signal_cache (or the client's cache clear/reconnect method) so the suite detects cache/backend divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/retry.rs`:
- Around line 178-189: The guard that bails out when a device is missing uses
has_device(&sender_user, sender_device_id) but only checks the original
participant_jid.user, which can falsely miss a LID-backed cached record that was
stored under the backend namespace resolved by resolve_encryption_jid; update
the check in the retry handling (around resolved_jid, participant_jid,
sender_device_id) to probe both the raw user and the resolved user (i.e., call
has_device with participant_jid.user and also with resolved_jid.user) and only
return early if both checks fail so process_retry_key_bundle() / resend logic
can still run for namespace-mapped devices.
---
Duplicate comments:
In `@tests/e2e/tests/lid_sessions.rs`:
- Around line 195-198: The info logs currently print real identifiers
(lid_sessions via "{lid_sessions:?}", and variables lid_addr and pn_addr) which
leaks PII; update every log site (e.g., the info! call referencing lid_sessions,
and the places around lid_addr and pn_addr noted) to either log only counts
(lid_sessions.len()) or map each address through the existing mask_addr() helper
before formatting (e.g., lid_sessions.iter().map(|a|
mask_addr(a)).collect::<Vec<_>>() or mask_addr(&lid_addr), mask_addr(&pn_addr));
ensure mask_addr is in scope and replace any "{...:?}" usages that would emit
raw JIDs/phone numbers with the masked values or counts consistently across the
file (also at the other ranges referenced).
- Around line 33-50: scan_sessions currently only queries the
persistence_manager().backend() via backend.get_session, which lets callers pass
while Client.signal_cache remains stale; update scan_sessions to either force a
live-store check or clear/reconnect the client cache before the loop: add an
argument to accept the test Client (or a cache-control helper) and call the
client's cache clear or reconnect method (eg. Client.clear_signal_cache() /
Client.reconnect()) or, for each addr, also query the live store via the
client's live-session lookup and assert it matches backend.get_session(addr);
ensure you reference scan_sessions, backend.get_session, and Client.signal_cache
(or the client's cache clear/reconnect method) so the suite detects
cache/backend divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c0e12f5-61e4-46e4-9344-7e20388a9142
📒 Files selected for processing (2)
src/retry.rstests/e2e/tests/lid_sessions.rs
resolve_encryption_jid result stored as resolved_jid instead of rebinding participant_jid, so PN-addressed groups get the correct participant namespace in retry stanzas. Also use is_same_user_as for is_peer check and mask remaining raw JIDs in test logs.
ee76654 to
1933a8d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1933a8d3bc
ℹ️ 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".
| // Try to process key bundle if present | ||
| let key_bundle_result = self | ||
| .process_retry_key_bundle(node, &participant_jid, is_peer) | ||
| .process_retry_key_bundle(node, &resolved_jid, is_peer) |
There was a problem hiding this comment.
Passing resolved_jid into process_retry_key_bundle makes the registration-ID guard read only the LID session, so when a peer device still has its existing session stored under PN (the exact legacy state this PR is migrating), existing_reg_id is treated as missing and the peer reinstall/change check is skipped. In that PN-only scenario, a retry from our own reinstalled device can be accepted instead of rejected, weakening the intended peer-device protection path.
Useful? React with 👍 / 👎.
1 participant
Summary
SessionNotFounderrors when receiving messages from the primary phone. The session was stored under a PN address from an old pairing, but the phone now sends from a LID address. Aligns with WA Web'screateSignalAddress()which always resolves PN→LID before any session operation.SessionNotFoundhandler, so existing databases are fixed without re-pairing.Changes
Core fix: PN→LID session migration (
src/client/lid_pn.rs)migrate_signal_sessions_on_lid_discovery()— scans PN sessions for a user (devices 0-99), migrates them to LID addresses. Identity migration runs independently of session existence (survives session deletion and re-establishment). Identity keys use protocol address format (with.0suffix) matching the Signal store backend.Login-time migration (
src/client/sessions.rs)establish_primary_phone_session_immediate()rewritten: checks LID session → cleans stale PN | migrates PN→LID | establishes fresh viafetch_and_establish_sessions(bypasseswait_for_offline_delivery_endsince we're at login).On-the-fly migration in decryption (
src/message.rs)try_pn_to_lid_migration_decrypt()helper: onSessionNotFoundfor a LID address, attempts PN→LID migration, reloads session+identity into signal cache, retries decryption. HandlesDuplicatedMessagegracefully in the post-migration retry path (silently ignored). Falls back to retry receipt only if migration doesn't help.sender_encryption_jidblock intocache_lid_pn_from_message()+resolve_encryption_jid().Retry path (
src/retry.rs)resolve_encryption_jid()applied early inhandle_retry_receiptso all downstream session operations (key bundle processing, registration ID checks, base key collision detection, session deletion) use the resolved LID address.Self-message sender_alt (
wacore/src/messages.rs)parse_message_infonow populatessender_altfor self-messages from the known own PN↔LID pair, so the LID-PN cache warms from own-device messages.Test plan
tests/e2e/tests/lid_sessions.rs(JIDs masked in logs):test_sessions_stored_under_lid_not_pn— bidirectional check, both sides LID-onlytest_multiple_sends_stay_lid_only— 5 sequential sends don't regress to PNtest_stale_pn_session_does_not_break_lid_messaging— injected stale PN doesn't break LIDtest_lid_session_survives_reconnect— LID sessions survive DB reload, no PN creeptest_own_device_0_has_lid_session_after_login— own device 0 LID session + no PN sessiontest_no_undecryptable_events_during_messaging— noUndecryptableMessageeventstest_pn_only_session_causes_undecryptable_on_lid_lookup— reproduces and fixes the production bug: injects PN-only session, reconnects, verifies message decrypts via on-the-fly migration, asserts noUndecryptableMessage, confirms session is under LID after fixsender_altassertions)Summary by CodeRabbit
New Features
Bug Fixes
Tests