perf: reduce duplication and unnecessary allocations - #499
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors string handling and memory allocation patterns across the codebase by consolidating waiter-resolution logic, passing borrowed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 20s —— View job Code Review:
|
Bencher Report
DRY improvements: - Unify NodeWaiter/SentNodeWaiter into single type with shared resolve_waiters() - Extract build_revoke_message() helper (3 identical constructions → 1) - Replace SenderKeyName::new(x.to_string(), y.to_string()) with from_jid() (4 sites) - Use existing notification_timestamp() helper instead of copy-pasted body (2 sites) Allocation reductions: - Use ProtocolAddress::as_str() instead of .to_string() for session lock keys - Return Vec<ProtocolAddress> from build_session_lock_keys() instead of Vec<String> - Use NodeValue::to_jid() instead of to_string().parse::<Jid>() in group send path - Borrow Jid keys in device encryption HashMap (HashMap<&Jid, Jid>) - Add From<&Jid> for NodeValue and remove ~36 unnecessary .clone() calls on Jid values passed to NodeBuilder::attr() across 25 files - Pass Jids directly to NodeBuilder::attr() as NodeValue::Jid (compact encoding) - Use as_str() over to_string() for presence timestamp parsing - Use static string for flush_signal_cache_logged context - Simplify build_session_lock_keys sort/dedup (ProtocolAddress implements Ord)
dedb8ba to
6d88e53
Compare
![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
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)
392-415: 🧹 Nitpick | 🔵 TrivialOnly store rewritten encryption JIDs in
jid_to_encryption_jid.Line 392 and the no-rewrite branch below still clone and insert identity mappings, but both later consumers already fall back to
device_jidwhen the map misses. Skipping those inserts keeps the common direct-session path actually zero-copy.♻️ Suggested change
if stores .session_store .load_session(&signal_address) .await? .is_some() { // Session exists under direct address, use it - jid_to_encryption_jid.insert(device_jid, device_jid.clone()); continue; } // No session found - need to fetch prekeys and create session. // Keep device_jid for prekey fetch (server returns bundles keyed by this), - // but normalize to LID for the actual session creation. - let encryption_jid = if device_jid.is_pn() { - if let Some(lid_user) = resolver.get_lid_for_phone(&device_jid.user).await { - let lid_jid = Jid::lid_device(lid_user, device_jid.device); - log::debug!( - "Will create LID session {} for PN {} (no existing session)", - lid_jid, - device_jid - ); - lid_jid - } else { - device_jid.clone() - } - } else { - device_jid.clone() - }; - jid_to_encryption_jid.insert(device_jid, encryption_jid); + // but normalize to LID for the actual session creation when needed. + if device_jid.is_pn() + && let Some(lid_user) = resolver.get_lid_for_phone(&device_jid.user).await + { + let lid_jid = Jid::lid_device(lid_user, device_jid.device); + log::debug!( + "Will create LID session {} for PN {} (no existing session)", + lid_jid, + device_jid + ); + jid_to_encryption_jid.insert(device_jid, lid_jid); + } // Use original device_jid for prekey fetch (HashMap key match) jids_needing_prekeys.push(device_jid.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/send.rs` around lines 392 - 415, The code currently inserts identity mappings into jid_to_encryption_jid for device_jid clones even when no rewrite occurs, wasting copies; change the logic in the block that computes encryption_jid (which uses resolver.get_lid_for_phone and Jid::lid_device) to only call jid_to_encryption_jid.insert(device_jid, encryption_jid) when encryption_jid != device_jid (i.e., a LID rewrite was produced); leave the original device_jid available and used for prekey fetch and do not insert the identity mapping so the common direct-session path stays zero-copy.
🤖 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 894-895: The helper notification_timestamp currently casts a u64
to i64 with `as`, causing wraparound for values > i64::MAX; change
notification_timestamp to perform a checked conversion (use i64::try_from on the
optional_u64("t") value), then build the chrono::DateTime only if the conversion
succeeds, otherwise fall back to chrono::Utc::now(); update the function body
used by callers (notification_timestamp) so it chains optional_u64("t") ->
i64::try_from(...).ok() -> DateTime creation and unwraps with Utc::now() on
failure.
---
Outside diff comments:
In `@wacore/src/send.rs`:
- Around line 392-415: The code currently inserts identity mappings into
jid_to_encryption_jid for device_jid clones even when no rewrite occurs, wasting
copies; change the logic in the block that computes encryption_jid (which uses
resolver.get_lid_for_phone and Jid::lid_device) to only call
jid_to_encryption_jid.insert(device_jid, encryption_jid) when encryption_jid !=
device_jid (i.e., a LID rewrite was produced); leave the original device_jid
available and used for prekey fetch and do not insert the identity mapping so
the common direct-session path stays zero-copy.
🪄 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: 5b38e5fb-c4a3-4000-97a2-4b07f1285bd3
📒 Files selected for processing (13)
src/client.rssrc/features/signal.rssrc/handlers/notification.rssrc/handlers/presence.rssrc/message.rssrc/retry.rssrc/send.rswacore/binary/src/node.rswacore/src/iq/dirty.rswacore/src/iq/usync.rswacore/src/media_retry.rswacore/src/send.rswacore/src/usync.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/message.rs`:
- Line 581: The call to self.flush_signal_cache_logged("message dispatch").await
loses per-message correlation in logs; modify the flush_signal_cache_logged call
to accept and forward the message ID (or reference to the message's correlation
id) so logs include that context without allocating (e.g., add a parameter like
message_id: &str or &MessageId and pass the existing id from the current message
before awaiting). Update the function signature of flush_signal_cache_logged and
its callers (and log statements inside it) to accept this id reference and use
it in error/debug messages while preserving zero-allocation usage.
🪄 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: f0779057-4009-48ff-a29a-e3041a4937be
📒 Files selected for processing (25)
src/client.rssrc/features/chatstate.rssrc/features/newsletter.rssrc/features/presence.rssrc/features/signal.rssrc/handlers/notification.rssrc/handlers/presence.rssrc/message.rssrc/receipt.rssrc/retry.rssrc/send.rswacore/binary/src/node.rswacore/src/iq/business.rswacore/src/iq/devices.rswacore/src/iq/dirty.rswacore/src/iq/groups.rswacore/src/iq/privacy.rswacore/src/iq/tctoken.rswacore/src/iq/usync.rswacore/src/media_retry.rswacore/src/pair.rswacore/src/prekeys.rswacore/src/request.rswacore/src/send.rswacore/src/usync.rs
| // Flush cached Signal state to DB (matches WA Web's flushBufferToDiskIfNotMemOnlyMode) | ||
| self.flush_signal_cache_logged(&format!("message {}", info.id)) | ||
| .await; | ||
| self.flush_signal_cache_logged("message dispatch").await; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve per-message flush context without reintroducing allocations.
Line 581 drops message-level correlation in error logs. You can keep zero-allocation behavior and retain debuggability by passing the message ID directly.
Suggested tweak
- self.flush_signal_cache_logged("message dispatch").await;
+ self.flush_signal_cache_logged(info.id.as_str()).await;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.flush_signal_cache_logged("message dispatch").await; | |
| self.flush_signal_cache_logged(info.id.as_str()).await; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` at line 581, The call to
self.flush_signal_cache_logged("message dispatch").await loses per-message
correlation in logs; modify the flush_signal_cache_logged call to accept and
forward the message ID (or reference to the message's correlation id) so logs
include that context without allocating (e.g., add a parameter like message_id:
&str or &MessageId and pass the existing id from the current message before
awaiting). Update the function signature of flush_signal_cache_logged and its
callers (and log statements inside it) to accept this id reference and use it in
error/debug messages while preserving zero-allocation usage.
Lock key optimization (benchmarked: 1.3-2.8x faster, 49-95% fewer RAM hits): - Sort Vec<Jid> by fields directly instead of allocating Vec<ProtocolAddress> - Add write_protocol_address_to() and cmp_for_lock_order() to wacore JidExt - Callers reuse a single String buffer for lock lookups (1 alloc vs 2N) - DRY: extracted mapped_server() shared by both new helpers and existing to_signal_address_string() flush_signal_cache_logged: - Accept optional message ID parameter for per-message log correlation - Zero-allocation: passes &str reference from existing MessageInfo.id - All other callers pass None (no behavior change) Also: add iai-callgrind benchmark for lock key approaches
![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: 101c23d7ac
ℹ️ 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".
| keys.sort_unstable_by(wacore::types::jid::cmp_for_lock_order); | ||
| keys.dedup(); |
There was a problem hiding this comment.
build_session_lock_keys() now sorts with cmp_for_lock_order and then calls dedup() on Vec<Jid>, but the lock key used later is produced by write_protocol_address_to() (which drops distinctions like s.whatsapp.net vs c.us, and agent/integrator fields). That means two different Jid values can survive dedup() while still mapping to the same session-lock key, so the send path can push the same mutex twice and then deadlock when locking guards sequentially (second lock().await waits on the first guard held in the same task).
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 (2)
src/retry.rs (1)
225-237:⚠️ Potential issue | 🟠 MajorTake the session lock around the retry invalidation path.
This registration-ID mismatch branch reads and deletes the cached session without
session_lock_for(). A concurrent encrypt/decrypt for the same sender can race this teardown and recreate the stale session you're trying to discard.🔒 Minimal fix shape
let signal_address = resolved_jid.to_protocol_address(); + let session_lock = self.session_lock_for(signal_address.as_str()).await; + let _session_guard = session_lock.lock().await; let device_store = self.persistence_manager.get_device_arc().await; let device_guard = device_store.read().await;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/retry.rs` around lines 225 - 237, The reg-ID mismatch branch reads and deletes the cached session without holding the per-sender lock; acquire the session lock via session_locks.session_lock_for(&signal_address).await (same lock used for encrypt/decrypt) before reading session.remote_registration_id() and performing self.signal_cache.delete_session(...) and self.flush_signal_cache_logged(...).await, hold that lock for the whole invalidation path so concurrent encrypt/decrypt cannot recreate the stale session; release the lock after flush completes. Ensure you use the existing session_locks construct (not message_enqueue_locks) and keep references to signal_address, stored_reg_id and received_reg_id when logging.src/handlers/notification.rs (1)
373-389:⚠️ Potential issue | 🟠 MajorSerialize identity-change teardown with
session_lock_for().This branch still deletes the pairwise session and identity outside the per-sender mutex. A concurrent encrypt/decrypt on the same address can race this invalidation and repopulate stale state after the identity-change cleanup. Please mirror
Signal::delete_sessions()here.🔒 Minimal fix shape
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 session_lock = client.session_lock_for(addr.as_str()).await; + let _session_guard = session_lock.lock().await; + client.signal_cache.delete_session(&addr).await; + client.signal_cache.delete_identity(&addr).await; + }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/handlers/notification.rs` around lines 373 - 389, The delete operations for pairwise session, identity and per-sender sender-keys must be performed under the per-sender mutex—wrap the teardown in session_lock_for() to serialize with concurrent encrypt/decrypt. Concretely: after resolving the sender addr via resolve_encryption_jid(&from_jid) acquire the session lock (use session_lock_for(&addr).await) and inside that lock call signal_cache.delete_session(&addr).await and signal_cache.delete_identity(&addr).await; similarly, for each own_jid compute the sender-key address (own_jid.to_protocol_address()) and acquire session_lock_for(&that_addr).await before calling signal_cache.delete_sender_key(sk_name.cache_key()).await (mirror the locking pattern used in Signal::delete_sessions()). Keep client.flush_signal_cache_logged("identity change", None).await outside or after releasing the per-sender locks.
🤖 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/send.rs`:
- Line 733: The call to build_revoke_message unnecessarily clones message_id;
since message_id is unused afterwards, remove the allocation by passing
message_id by value (i.e., replace message_id.clone() with message_id) when
constructing revoke_message in the build_revoke_message call, ensuring the
function signature of build_revoke_message accepts ownership of message_id and
updating any calling context if needed.
---
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 373-389: The delete operations for pairwise session, identity and
per-sender sender-keys must be performed under the per-sender mutex—wrap the
teardown in session_lock_for() to serialize with concurrent encrypt/decrypt.
Concretely: after resolving the sender addr via
resolve_encryption_jid(&from_jid) acquire the session lock (use
session_lock_for(&addr).await) and inside that lock call
signal_cache.delete_session(&addr).await and
signal_cache.delete_identity(&addr).await; similarly, for each own_jid compute
the sender-key address (own_jid.to_protocol_address()) and acquire
session_lock_for(&that_addr).await before calling
signal_cache.delete_sender_key(sk_name.cache_key()).await (mirror the locking
pattern used in Signal::delete_sessions()). Keep
client.flush_signal_cache_logged("identity change", None).await outside or after
releasing the per-sender locks.
In `@src/retry.rs`:
- Around line 225-237: The reg-ID mismatch branch reads and deletes the cached
session without holding the per-sender lock; acquire the session lock via
session_locks.session_lock_for(&signal_address).await (same lock used for
encrypt/decrypt) before reading session.remote_registration_id() and performing
self.signal_cache.delete_session(...) and
self.flush_signal_cache_logged(...).await, hold that lock for the whole
invalidation path so concurrent encrypt/decrypt cannot recreate the stale
session; release the lock after flush completes. Ensure you use the existing
session_locks construct (not message_enqueue_locks) and keep references to
signal_address, stored_reg_id and received_reg_id when logging.
🪄 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: 8c8a4815-31ed-4564-9609-5f270a98a17e
📒 Files selected for processing (10)
src/client.rssrc/client/device_registry.rssrc/features/signal.rssrc/handlers/notification.rssrc/message.rssrc/retry.rssrc/send.rswacore/Cargo.tomlwacore/benches/lock_key_benchmark.rswacore/src/types/jid.rs
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 7s —— View job Code Review:
|
- Fix dedup_by to use cmp_for_lock_order (not PartialEq) so JIDs that differ only in agent/integrator but map to the same lock key are correctly deduplicated (prevents double-locking the same mutex) - Remove unnecessary message_id.clone() in revoke_message (moved instead) - Acquire session lock before deleting session in identity change handler (mirrors Signal::delete_sessions pattern, prevents race with concurrent encrypt/decrypt recreating the stale session) - Acquire session lock before deleting session in reg-ID mismatch handler (same race condition)
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 31s —— View job Code Review:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/notification.rs (1)
902-903:⚠️ Potential issue | 🟡 Minor
notification_timestamp()still needs a checked cast.These new call sites now depend on a helper that still does
u64 as i64. Oversizedtvalues become negative seconds, sofrom_timestamp()can return a bogus timestamp instead of taking theUtc::now()fallback.🛠️ Proposed fix
fn notification_timestamp(node: &Node) -> chrono::DateTime<chrono::Utc> { node.attrs() .optional_u64("t") - .map(|t| chrono::DateTime::from_timestamp(t as i64, 0).unwrap_or_else(chrono::Utc::now)) + .and_then(|t| i64::try_from(t).ok()) + .and_then(|t| chrono::DateTime::from_timestamp(t, 0)) .unwrap_or_else(chrono::Utc::now) }Also applies to: 992-992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 902 - 903, The call to notification_timestamp(node) relies on notification_timestamp performing an unchecked u64 as i64 cast which can produce negative seconds for oversized values; update notification_timestamp to perform a checked conversion (use i64::try_from or TryInto) and handle failure by returning the Utc::now() fallback (or saturating/clamping to i64::MAX if intended), ensuring callers like notification_timestamp(...) and other call sites no longer receive bogus negative seconds for from_timestamp(); update the function signature/return to reflect the safe path so callers simply use the returned chrono::DateTime<Utc>.
🤖 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/retry.rs`:
- Around line 997-999: The test helper build_retry_receipt sets the "to"
attribute using info.source.sender which diverges from production group routing
that uses info.source.chat; update build_retry_receipt (and the similar
occurrences around the other diffs) to set .attr("to", &info.source.chat) when
info.source represents a group chat (or otherwise mirror the production routing
decision), preserving id and type attributes and keeping sender usage only for
non-group routes so tests exercise group-addressing the same way as production.
---
Duplicate comments:
In `@src/handlers/notification.rs`:
- Around line 902-903: The call to notification_timestamp(node) relies on
notification_timestamp performing an unchecked u64 as i64 cast which can produce
negative seconds for oversized values; update notification_timestamp to perform
a checked conversion (use i64::try_from or TryInto) and handle failure by
returning the Utc::now() fallback (or saturating/clamping to i64::MAX if
intended), ensuring callers like notification_timestamp(...) and other call
sites no longer receive bogus negative seconds for from_timestamp(); update the
function signature/return to reflect the safe path so callers simply use the
returned chrono::DateTime<Utc>.
🪄 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: 96dd26f0-7f0e-47ce-8583-07b349d2a37e
📒 Files selected for processing (3)
src/handlers/notification.rssrc/retry.rssrc/send.rs
- Fix test helper build_retry_receipt to route group retry receipts to info.source.chat (not sender), mirroring production logic at line 774 - Use i64::try_from instead of unchecked u64 as i64 in notification_timestamp (falls back to Utc::now on overflow instead of wrapping to negative)
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)
217-223:⚠️ Potential issue | 🟠 MajorAcquire the session lock before reading the cached session.
Line 235 starts the lock, but Lines 217-228 already read and compare the session state. A concurrent encrypt/decrypt can recreate or replace that session in the gap, and this branch will then delete the fresh state based on stale data.
Suggested fix
- let device_store = self.persistence_manager.get_device_arc().await; - let device_guard = device_store.read().await; + let lock = self.session_lock_for(signal_address.as_str()).await; + let _guard = lock.lock().await; + let device_store = self.persistence_manager.get_device_arc().await; + let device_guard = device_store.read().await; // Read session through cache to get consistent state let session = self .signal_cache .get_session(&signal_address, &*device_guard.backend) .await @@ - let lock = self.session_lock_for(signal_address.as_str()).await; - let _guard = lock.lock().await; self.signal_cache.delete_session(&signal_address).await; drop(_guard);Based on learnings: "Use
session_locksto serialize per-sender Signal encrypt/decrypt operations andmessage_enqueue_locksto serialize per-chat incoming message processing".Also applies to: 225-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 217 - 223, Currently code calls signal_cache.get_session(...) (via signal_cache.get_session and device_guard) before acquiring the per-sender session lock, which allows a concurrent encrypt/decrypt to replace the session and cause stale-state deletion; fix by acquiring the per-sender session lock from session_locks (serialize per-sender Signal ops) before calling signal_cache.get_session and hold that lock until after you finish comparing/deleting/setting session state (also apply same pattern around the other block using message_enqueue_locks for per-chat processing), i.e., move the session_locks acquisition so it surrounds the calls to signal_cache.get_session and the subsequent logic that may drop or replace the session to prevent races.
♻️ Duplicate comments (1)
src/retry.rs (1)
996-1004:⚠️ Potential issue | 🟡 MinorFinish covering the routing fix with
toassertions.The helper now mirrors production, but Line 1044 and Line 1103 still never check
to. The group-routing regression fixed here can come back without failing this test.Suggested assertions
let node = build_retry_receipt(&device_sync_info, &our_pn, &our_lid); + assert_eq!( + node.attrs.get("to").map(|v| v == "100000000000001@lid"), + Some(true), + "Device sync DM should target the sender JID" + ); assert_eq!( node.attrs .get("recipient") .map(|v| v == "200000000000002@lid"), @@ let node = build_retry_receipt(&group_info, &our_pn, &our_lid); + assert_eq!( + node.attrs.get("to").map(|v| v == "123456789@g.us"), + Some(true), + "Group retry should target the chat JID" + ); assert!( node.attrs.get("participant").is_some(), "Group should have participant" );Also applies to: 1044-1059, 1103-1115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/retry.rs` around lines 996 - 1004, The tests still don't assert the "to" routing set by receipt_to; update the test assertions that inspect the Node built by NodeBuilder::new("receipt") (the builder with .attr("to", receipt_to).attr("id", info.id.clone())) to explicitly check the "to" attribute matches the expected value: when info.source.is_group is true assert "to" == info.source.chat, and when false assert "to" == info.source.sender; add these "to" checks alongside the existing id/from/type assertions in the affected test cases so the group-routing regression will fail the test if it reappears.
🤖 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 217-223: Currently code calls signal_cache.get_session(...) (via
signal_cache.get_session and device_guard) before acquiring the per-sender
session lock, which allows a concurrent encrypt/decrypt to replace the session
and cause stale-state deletion; fix by acquiring the per-sender session lock
from session_locks (serialize per-sender Signal ops) before calling
signal_cache.get_session and hold that lock until after you finish
comparing/deleting/setting session state (also apply same pattern around the
other block using message_enqueue_locks for per-chat processing), i.e., move the
session_locks acquisition so it surrounds the calls to signal_cache.get_session
and the subsequent logic that may drop or replace the session to prevent races.
---
Duplicate comments:
In `@src/retry.rs`:
- Around line 996-1004: The tests still don't assert the "to" routing set by
receipt_to; update the test assertions that inspect the Node built by
NodeBuilder::new("receipt") (the builder with .attr("to", receipt_to).attr("id",
info.id.clone())) to explicitly check the "to" attribute matches the expected
value: when info.source.is_group is true assert "to" == info.source.chat, and
when false assert "to" == info.source.sender; add these "to" checks alongside
the existing id/from/type assertions in the affected test cases so the
group-routing regression will fail the test if it reappears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34aee1e4-f207-434a-8702-098656aceac9
📒 Files selected for processing (2)
src/handlers/notification.rssrc/retry.rs
1 participant
Summary
29 files changed, net -46 lines
DRY
NodeWaiter/SentNodeWaiterinto single type with sharedresolve_waiters()helperbuild_revoke_message()— 3 identical constructions → 1 functionSenderKeyName::from_jid()— 4 call sites eliminated manual.to_string()pairsnotification_timestamp()— 2 sites had copy-pasted its bodymapped_server()— shared byto_signal_address_string(),write_protocol_address_to(), andcmp_for_lock_order()Allocation reductions
Vec<Jid>by fields directly instead of allocatingVec<ProtocolAddress>(2 Strings per device). Callers reuse a singleStringbuffer for lock lookups. Benchmarked with iai-callgrind: 1.3-2.8x faster, 49-95% fewer RAM hitsFrom<&Jid> for NodeValue— new impl enabling ~36.clone()removals across IQ specs, stanzas, receipts, retry, features, and request buildingNodeBuilder::attr()asNodeValue::Jid— compact binary JID_PAIR/AD_JID encoding instead of stringifying (verified against WA Web'sWap.js)NodeValue::to_jid()instead ofto_string().parse::<Jid>()in group send participant dedupHashMap<&Jid, Jid>for device encryption map — borrows keys from input sliceProtocolAddress::as_str()instead of.to_string()for session lock keysas_str()overto_string()for presence timestamp parsingmessage_idinstead of.clone()inrevoke_message()(value unused after)Correctness fixes
dedup_by(cmp_for_lock_order)instead ofdedup()— JIDs that differ only inagent/integratorbut map to the same lock key are now correctly deduplicated (prevents double-locking the same mutex)delete_session+delete_identitynow held under per-sender session lock (mirrorsSignal::delete_sessions()pattern, prevents race with concurrent encrypt/decrypt)Logging
flush_signal_cache_loggedaccepts optional message ID for per-message log correlation without allocation (borrows existing&info.id)Verified against WhatsApp Web
JID_PAIR/AD_JIDtokens (matchesWap.js:159-180)RevokeMessageUtils.js:78-84attrInt("t")patterndeleteRemoteInfo()dual-delete pattern fromSessionApi.jsTest plan
cargo fmt --allcargo clippy --all --tests— zero warningscargo test --workspace --exclude e2e-tests— 1119 tests passSummary by CodeRabbit