fix: complete identity change handler to match WA Web - #490
Conversation
- Delete primary device session + identity key so fresh session can be established (WA Web's deleteRemoteInfo) - Delete status@broadcast sender key for forward secrecy on next status send (WA Web's markStatusSenderKeyRotate) - Spawn background ensure_e2e_sessions to proactively re-establish session (self-defers when offline) - Clean up verbose comments
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 15s —— View job Code Review: PR #490
Overall this is a solid PR that closes the remaining gaps from #489. The three new operations (primary session deletion, status sender key rotation, background session re-establishment) align well with the WA Web reference. A few observations: Potential Issue: Missing flush after sender key deletionIn the primary session deletion block (lines 367–377), you correctly call If the process crashes between these two points, you'd have the primary session deleted (flushed) but the status sender key still alive on disk — a partial state. Consider adding a second flush after the sender key loop, or combining both blocks before a single flush. Minor: Duplicate
|
📝 WalkthroughWalkthroughhandle_identity_change in Changes
Sequence DiagramsequenceDiagram
participant Notif as Notification Handler
participant Cache as Signal Cache
participant Device as Device Cache
participant Event as Event Dispatcher
participant BG as Background Task
participant E2E as E2E Session Manager
Notif->>Cache: clear_device_record(jid)
Notif->>Cache: delete_primary_session(jid)
Notif->>Cache: delete_identity(jid)
Notif->>Cache: delete_sender_keys("status@broadcast", jid)
Notif->>Cache: flush()
Note right of Cache: warn on flush failure
Notif->>Device: invalidate_device_cache(jid)
Notif->>Event: dispatch(Event::IdentityChange)
Notif->>BG: spawn detached ensure_e2e_sessions([session_jid])
BG->>E2E: ensure_e2e_sessions -> establish sessions (self-defer if offline)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: fbff40e8db
ℹ️ 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".
| client | ||
| .signal_cache | ||
| .delete_sender_key(sk_name.cache_key()) | ||
| .await; |
There was a problem hiding this comment.
delete_sender_key() only marks an in-memory cache entry dirty, but this handler never calls flush_signal_cache() after deleting status@broadcast keys. If ensure_e2e_sessions cannot establish a session (e.g., missing prekeys/offline) or the process restarts before another flush path runs, the old sender key remains in the DB and can be reused after restart, which breaks the intended forward-secrecy rotation on identity change.
Useful? React with 👍 / 👎.
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/handlers/notification.rs`:
- Around line 407-417: Multiple concurrent identity-change notifications can
spawn duplicate background tasks calling
client_clone.ensure_e2e_sessions(&[session_jid]); add a per-user dedupe lock
(similar to prekey_upload_lock) keyed by session_jid or user JID to avoid
spawning redundant tasks: before runtime.spawn, acquire or check a short-lived
in-memory Mutex/HashSet entry for that jid to ensure only one detached task is
active for that jid, release the entry when the spawned task completes (or use a
oneshot/future map) so subsequent notifications can spawn a new task; update the
logic around client.clone(), runtime.spawn, ensure_e2e_sessions and detach to
consult this dedupe structure.
- Around line 2079-2114: The test test_identity_change_deletes_primary_session
currently only asserts the session was deleted; add an assertion that the
identity was also removed after handle_notification_impl runs. After computing
backend (let backend = client.persistence_manager.backend()), call the
appropriate signal_cache identity check (e.g.
client.signal_cache.has_identity(&addr, &*backend).await.unwrap() or
client.signal_cache.get_identity(&addr, &*backend).await.unwrap() == None) and
assert the identity is gone, referencing the existing use of
client.signal_cache, handle_notification_impl, and addr/target_jid to locate
where to add this assertion.
- Around line 379-395: The sender key deletions performed via
client.signal_cache.delete_sender_key(...) inside the block that constructs
SenderKeyName for "status@broadcast" are not followed by a persistence flush;
add a call to the existing flush method (e.g. client.flush_signal_cache() or the
project's canonical flush_signal_cache function) immediately after the for-loop
to ensure the deletions are persisted like the preceding
delete_session/delete_identity calls (refer to delete_session, delete_identity
usages and flush_signal_cache patterns in src/message.rs, src/retry.rs,
src/features/signal.rs).
🪄 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: 3ca8ab9c-c9f5-4443-b44c-31bfa4743673
📒 Files selected for processing (1)
src/handlers/notification.rs
Merge session/identity deletion and status sender key deletion into a single block with one flush, ensuring the sender key tombstone is persisted before the handler returns.
|
@claude review |
|
Claude finished @jlucaso1's task in 34s —— View job
Code Review: PR #490 (re-review after fixes)Both issues from my previous review have been addressed:
No remaining functional concerns. The code is clean and well-tested. LGTM. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/notification.rs`:
- Around line 2128-2133: The test is directly mutating Device.pn via
client.persistence_manager.modify_device, which bypasses invariants; instead
construct the appropriate DeviceCommand to set the pn (e.g., a command that
updates the pn field) and send it through PersistenceManager::process_command
(using the same process_command() path), then verify the change by calling
client.persistence_manager.get_device_snapshot() rather than reading the device
directly; replace the modify_device(...) closure call with creating the
DeviceCommand, processing it, and reading the snapshot to exercise the supported
surface.
- Around line 367-389: The code uses a stale device_snapshot when rotating
status@broadcast sender keys which can miss current PN/LID after awaiting;
before building SenderKeyName and calling client.signal_cache.delete_sender_key,
re-fetch or refresh the local device snapshot (the source used to populate
device_snapshot.pn and device_snapshot.lid) so that you iterate over the
up-to-date PN/LID values; specifically, obtain an updated snapshot after the
await boundaries (before the for own_jid in
device_snapshot.pn.iter().chain(device_snapshot.lid.iter()) loop) and use that
fresh list to construct SenderKeyName::new(status_group.to_string(),
own_jid.to_protocol_address().to_string()) and call
client.signal_cache.delete_sender_key(...) to ensure you delete the active
sender keys.
🪄 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: 9c8b349e-088d-4fd7-aaaf-4b2859e3049f
📒 Files selected for processing (1)
src/handlers/notification.rs
| // Delete primary session + identity so a fresh session can be established, | ||
| // and rotate status sender key for forward secrecy (clear_device_record only | ||
| // cleared device tracking, not the key itself). Single flush covers both. | ||
| { | ||
| use wacore::libsignal::store::sender_key_name::SenderKeyName; | ||
| use wacore::types::jid::JidExt; | ||
|
|
||
| let resolved = client.resolve_encryption_jid(&from_jid).await; | ||
| let addr = resolved.to_protocol_address(); | ||
| client.signal_cache.delete_session(&addr).await; | ||
| client.signal_cache.delete_identity(&addr).await; | ||
|
|
||
| let status_group = "status@broadcast"; | ||
| for own_jid in device_snapshot.pn.iter().chain(device_snapshot.lid.iter()) { | ||
| let sk_name = SenderKeyName::new( | ||
| status_group.to_string(), | ||
| own_jid.to_protocol_address().to_string(), | ||
| ); | ||
| client | ||
| .signal_cache | ||
| .delete_sender_key(sk_name.cache_key()) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
Refresh the local device snapshot before rotating status@broadcast sender keys.
device_snapshot is frozen at Line 341 and then reused here after several awaits to build the SenderKeyNames. If our PN/LID changes in that window, this loop deletes the old cache keys and leaves the active sender key intact, so the forward-secrecy rotation silently misses the current identity.
♻️ Proposed fix
- let status_group = "status@broadcast";
- for own_jid in device_snapshot.pn.iter().chain(device_snapshot.lid.iter()) {
+ let own_device_snapshot = client.persistence_manager.get_device_snapshot().await;
+ let status_group = "status@broadcast";
+ for own_jid in own_device_snapshot
+ .pn
+ .iter()
+ .chain(own_device_snapshot.lid.iter())
+ {
let sk_name = SenderKeyName::new(
status_group.to_string(),
own_jid.to_protocol_address().to_string(),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/notification.rs` around lines 367 - 389, The code uses a stale
device_snapshot when rotating status@broadcast sender keys which can miss
current PN/LID after awaiting; before building SenderKeyName and calling
client.signal_cache.delete_sender_key, re-fetch or refresh the local device
snapshot (the source used to populate device_snapshot.pn and
device_snapshot.lid) so that you iterate over the up-to-date PN/LID values;
specifically, obtain an updated snapshot after the await boundaries (before the
for own_jid in device_snapshot.pn.iter().chain(device_snapshot.lid.iter()) loop)
and use that fresh list to construct
SenderKeyName::new(status_group.to_string(),
own_jid.to_protocol_address().to_string()) and call
client.signal_cache.delete_sender_key(...) to ensure you delete the active
sender keys.
| client | ||
| .persistence_manager | ||
| .modify_device(|d| { | ||
| d.pn = Some(own_jid.clone()); | ||
| }) | ||
| .await; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid direct Device mutation in the new test setup.
This seeds pn through modify_device, which bypasses the same mutation path production code uses to preserve device-state invariants. Please set this up via DeviceCommand/process_command() so the test exercises the supported surface.
As per coding guidelines "Never modify Device state directly; use DeviceCommand + PersistenceManager::process_command() and read via get_device_snapshot()".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/notification.rs` around lines 2128 - 2133, The test is directly
mutating Device.pn via client.persistence_manager.modify_device, which bypasses
invariants; instead construct the appropriate DeviceCommand to set the pn (e.g.,
a command that updates the pn field) and send it through
PersistenceManager::process_command (using the same process_command() path),
then verify the change by calling
client.persistence_manager.get_device_snapshot() rather than reading the device
directly; replace the modify_device(...) closure call with creating the
DeviceCommand, processing it, and reading the snapshot to exercise the supported
surface.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aed40490c
ℹ️ 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".
| client.signal_cache.delete_session(&addr).await; | ||
| client.signal_cache.delete_identity(&addr).await; |
There was a problem hiding this comment.
This new primary-session reset path deletes session and identity directly without taking the per-address session mutex, so it is not serialized with in-flight encrypt/decrypt work that uses session_lock_for(...) (see the existing locked deletion pattern in SignalFeature::delete_sessions). If a send is already using the old session when this handler runs, it can repopulate that stale session after these deletes are flushed, and then ensure_e2e_sessions will treat the session as present and skip prekey re-establishment, effectively undoing the identity-change reset for that user.
Useful? React with 👍 / 👎.
1 participant
Summary
Completes the identity change handler from PR #489 with three remaining gaps verified against
WAWebHandleIdentityChange:deleteRemoteInfo)markStatusSenderKeyRotate)ensure_e2e_sessionsto proactively re-establish session (self-defers when offline viawait_for_offline_delivery_end)Test plan
test_identity_change_deletes_primary_session— session + identity removed from signal cachetest_identity_change_rotates_status_sender_key— status sender key deleted for forward secrecytest_identity_change_with_offline_attribute— offline notification processed without errorSummary by CodeRabbit
Bug Fixes
Tests