feat: lid pn mapping and edge route support - #181
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new LID↔phone-number in-memory cache with persistence bindings and DB migrations, migrates stanza handling from borrowed NodeRef to Arc with per-chat enqueue workers, introduces edge-routing pre-intro in handshake and a task-based Noise send pipeline, and wires LID-aware session resolution across send/receive flows. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements LID-to-Phone Number mapping and edge routing support to improve WhatsApp protocol compatibility and session management. The changes address critical issues with Signal protocol session handling when users have both phone number and LID identities.
Key Changes:
- Fixed Signal protocol address format to match WhatsApp Web's behavior (device encoded in name, device_id always 0)
- Implemented LID-PN bidirectional mapping cache with persistent storage
- Added edge routing support for optimized server reconnection
- Refactored message processing to use Arc-wrapped nodes instead of cloning
- Improved connection lifecycle management with generation tracking
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| wacore/src/types/jid.rs | Updated protocol address generation to match WhatsApp Web's format with device in name |
| wacore/src/usync.rs | Added LID mapping extraction from usync responses |
| wacore/src/send.rs | Implemented LID session lookup to reuse existing sessions instead of creating duplicates |
| src/lid_pn_cache.rs | New bidirectional cache for LID-PN mappings with timestamp-based conflict resolution |
| src/client.rs | Refactored node processing to use Arc, added LID cache warmup and connection generation tracking |
| storages/sqlite-storage/src/sqlite_store.rs | Added LID-PN mapping table, session/identity caching, and foreign key constraints |
| wacore/libsignal/src/protocol/state/session.rs | Optimized receiver chain lookups to avoid unnecessary cloning |
| wacore/libsignal/src/protocol/session_cipher.rs | Fixed identity trust handling for previous sessions to prevent UntrustedIdentity errors |
| src/message.rs | Updated message decryption to use LID addresses for session lookup when mapping is known |
| src/handlers/message.rs | Refactored to use per-chat worker queues with strict ordering guarantees |
| wacore/src/pair.rs | Disabled HMAC verification temporarily (marked with comment indicating incomplete implementation) |
| wacore/src/store/device.rs | Added edge_routing_info field for optimized reconnection |
| transports/tokio-transport/src/lib.rs | Added TLS certificate verification skip feature and custom connector support |
Comments suppressed due to low confidence (4)
wacore/src/pair.rs:1
- HMAC verification is disabled in pairing flow. This is a critical security issue - the commented code shows HMAC validation should verify the device identity signature. The variable is renamed to
_hmac_bytes(unused) and the verification is commented out, which could allow forged pairing requests. This should be re-enabled or documented with a clear explanation of why it's safe to skip.
wacore/src/pair.rs:1 - HMAC verification is disabled in pairing flow. This is a critical security issue - the commented code shows HMAC validation should verify the device identity signature. The variable is renamed to
_hmac_bytes(unused) and the verification is commented out, which could allow forged pairing requests. This should be re-enabled or documented with a clear explanation of why it's safe to skip.
storages/sqlite-storage/src/sqlite_store.rs:1 - The comment says 'Limit concurrent connections' but the pool_size was increased from 4 to 32. The comment is misleading - it should mention that the pool was increased to support higher concurrency, not reduced to limit it.
wacore/binary/Cargo.toml:1 - The
optional = trueflag was removed from the build-dependencies serde entry. This means serde is now always included in build dependencies even when not needed. The removal should be intentional - if the serde feature is always required for build scripts, this is correct, but it should be verified.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
wacore/libsignal/src/protocol/state/session.rs (2)
425-442: Avoid panics: replaceexpect(...)with an error return.
expect("called set_message_keys for a non-existent chain")can crash the process if state is corrupted or the API is misused. Since this already returnsResult<_, InvalidSessionError>, prefer a typed error.- let chain_idx = self - .get_receiver_chain_index(sender)? - .expect("called set_message_keys for a non-existent chain"); + let chain_idx = self + .get_receiver_chain_index(sender)? + .ok_or(InvalidSessionError("called set_message_keys for a non-existent chain"))?;
444-460: Same here: avoid panics inset_receiver_chain_key.
This should fail gracefully (and consistently with other session-structure validation) rather than aborting.- let chain_idx = self - .get_receiver_chain_index(sender)? - .expect("called set_receiver_chain_key for a non-existent chain"); + let chain_idx = self + .get_receiver_chain_index(sender)? + .ok_or(InvalidSessionError("called set_receiver_chain_key for a non-existent chain"))?;wacore/src/pair.rs (2)
125-154: Reinstate HMAC verification (pairing auth is currently bypassed).
Themac.verify_slice(...)check is commented out, so a maliciousdevice_identity_bytescan pass without the outer HMAC integrity check.Suggested fix (also avoids the “unused” issue by using
hmac_bytesagain):- let _hmac_bytes = hmac_container + let hmac_bytes = hmac_container .hmac .as_deref() .ok_or_else(|| PairCryptoError { code: 500, text: "internal-error", source: anyhow::anyhow!("HMAC container missing hmac"), })?; @@ if is_hosted_account { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // if mac.verify_slice(hmac_bytes).is_err() { - // return Err(PairCryptoError { - // code: 401, - // text: "hmac-mismatch", - // source: anyhow::anyhow!("HMAC mismatch"), - // }); - // } + if mac.verify_slice(hmac_bytes).is_err() { + return Err(PairCryptoError { + code: 401, + text: "hmac-mismatch", + source: anyhow::anyhow!("HMAC mismatch"), + }); + }
201-218: Fix RNG construction incalculate_signaturecall — current code is syntactically invalid.Line 212:
&mut rand::rngs::OsRng::unwrap_err(rand_core::OsRng)will not compile. Theunwrap_err()method is for Result types, but OsRng is not a Result.Correct approach for rand/rand_core 0.9:
+ let mut rng = rand_core::OsRng; let device_signature = device_state .identity_key .private_key .calculate_signature( &msg_to_sign, - &mut rand::rngs::OsRng::unwrap_err(rand_core::OsRng), + &mut rng, )wacore/binary/src/jid.rs (1)
224-259: Bug risk:hosted.lidisn’t included in agent-suppression (and likely should be inis_ad()).If
agent > 0ever appears on@hosted.lid, Display currently printsuser.<agent>:<device>@hosted.lid, which seems contrary to the intent of the parity fixes for@hosted/@lid.Proposed patch:
pub trait JidExt { @@ fn is_ad(&self) -> bool { self.device() > 0 && (self.server() == DEFAULT_USER_SERVER || self.server() == HIDDEN_USER_SERVER - || self.server() == HOSTED_SERVER) + || self.server() == HOSTED_SERVER + || self.server() == HOSTED_LID_SERVER) } @@ impl fmt::Display for Jid { @@ let server_str = self.server(); // Use trait method if server_str != DEFAULT_USER_SERVER && server_str != HIDDEN_USER_SERVER && server_str != HOSTED_SERVER + && server_str != HOSTED_LID_SERVER { write!(f, ".{}", self.agent)?; } @@ impl<'a> fmt::Display for JidRef<'a> { @@ let server_str = self.server(); // Use trait method if server_str != DEFAULT_USER_SERVER && server_str != HIDDEN_USER_SERVER && server_str != HOSTED_SERVER + && server_str != HOSTED_LID_SERVER { write!(f, ".{}", self.agent)?; }Also applies to: 488-519, 522-554
src/receipt.rs (1)
10-66: GoodArc<Node>migration for spawn safety; consider downgrading receipt metadata logs todebugto avoid leaking phone numbers.If logs are enabled in user environments,
fromcommonly embeds phone numbers. Suggest:-use log::info; +use log::{debug, info}; @@ - info!("Received receipt type '{receipt_type:?}' for message {id} from {from}"); + debug!("Received receipt type '{receipt_type:?}' for message {id} from {from}");src/handlers/notification.rs (1)
36-86: Potential perf + behavior issues: deepNodeclones for events, and "server_sync scheduling" not implemented.
Event::NotificationrequiresEventto beClone(used byBotEventHandlerinbot.rs), which meansnode.clone()performs a deep copy at dispatch time. The codebase hasSharedData<T>wrapper for exactly this pattern; consider usingSharedData<Node>inEvent::Notification, or changing toArc<Node>in theEventenum definition.- In
server_sync(lines 50–62), the log says "scheduling app state sync(s)" but only iterates and logs collection metadata. The actual sync should enqueueMajorSyncTask::AppStateSyncfor each collection (similar to howprocess_app_state_sync_taskis invoked frombot.rs), or rename the log to "discovered" to avoid misleading operators.src/retry.rs (1)
170-179: Likely broken RNG usage (rand::rngs::OsRng.unwrap_err()), please fix to a real CSPRNG instance.This looks like it won’t compile / isn’t the intended rand API. Suggest:
- let new_prekey_keypair = KeyPair::generate(&mut rand::rngs::OsRng.unwrap_err()); + let mut rng = rand::rngs::OsRng; + let new_prekey_keypair = KeyPair::generate(&mut rng);(Adjust if
KeyPair::generateexpects a different trait bound for yourwacore::libsignalversion.)src/message.rs (2)
393-393: Critical bug:unwrap_err()on RNG will panic on success.
OsRng.unwrap_err()callsResult::unwrap_err()which panics when theResultisOk. SinceOsRngtypically succeeds, this will panic at runtime. TheTryRngCoretrait'stry_fill_bytesreturnsResult<(), TryError>, and the pattern used elsewhere suggests this should beOsRngdirectly or use the appropriate RNG interface.Looking at line 350,
rand::rngs::OsRngis created withoutunwrap_err(), which is correct. Theunwrap_err()calls on lines 393 and 469 appear to be copy-paste errors.- &mut rng.unwrap_err(), + &mut rand::rngs::OsRng,
469-469: Same critical bug:unwrap_err()will panic.This is the same issue as line 393 - calling
unwrap_err()onOsRngwill panic when the RNG succeeds.- &mut rng.unwrap_err(), + &mut rand::rngs::OsRng,
🧹 Nitpick comments (15)
src/socket/noise_socket.rs (2)
12-12: Validate the throughput/latency tradeoff: mutex is held across encryption +transport.send().await.
Holdingsend_mutexacrossspawn_blockingcompletion and the awaited network send guarantees strict on-the-wire ordering, but it also forces a single-flight send pipeline (potential backpressure if the transport is slow). That may be exactly what you want, but it’s worth verifying this doesn’t regress high-throughput scenarios. Also, increasingINLINE_ENCRYPT_THRESHOLDto 128 KiB increases crypto work on the async executor; please validate/benchmark that this doesn’t starve other tasks (per guideline: heavy CPU →spawn_blocking).If you observe regressions, consider a dedicated ordered send queue (single sender task) so callers can enqueue frames quickly while preserving order (instead of holding a mutex across network I/O).
Also applies to: 57-60, 96-108, 132-134
5-5: Thesend_mutexcorrectly serializes sends, butencrypt_intois a public footgun.
send_mutexproperly ensures all frames sent viaencrypt_and_sendusewrite_counterin order. However,encrypt_into(line 41) is a public method that incrementswrite_counterwithout the lock—meaning if external code calls it, encrypts data, then sends those frames separately (or concurrently withencrypt_and_send), the counters will be out of order.Currently,
encrypt_intois unused in the codebase, so the actual sending remains correctly serialized. But since it's part of the public API, consider either:
- Making it private if not intended for external use, or
- Documenting that it must not be used for frames intended to be sent on this socket, only for data encryption where counter ordering doesn't matter.
wacore/libsignal/src/protocol/session_cipher.rs (1)
683-686: Debug logging includes sensitive key material.The debug logs output cryptographic key material (mac_key, root_key, ephemeral keys) in hexadecimal format. While this is valuable for debugging ratchet issues, it poses a risk if debug logging is accidentally enabled in production environments.
Ensure that:
- Debug logging is strictly disabled in production builds
- If debug logs are captured, they are stored securely and access is restricted
This is generally acceptable practice for cryptographic debugging, but worth noting for security awareness.
Also applies to: 755-768
transports/tokio-transport/src/lib.rs (1)
188-222: Revisit channel capacity bump to 10,000 (memory/backpressure).
If the consumer stalls, this can accumulate a lot of bufferedTransportEvent::DataReceived(Bytes)in memory. Consider making it configurable, or documenting why 10,000 is required vs applying backpressure sooner.src/store/signal_adapter.rs (1)
56-92: Avoid holdingtokio::RwLockguards across backend.await(contention risk).
Bothload_sessionandstore_sessionkeep thedevice.read().awaitguard while awaiting backend I/O. If possible, clone/extract the backend handle while under the lock, drop the guard, then await the backend call. This reduces lock coupling and improves concurrency.wacore/src/client/context.rs (2)
7-19: Avoid hard-coded JID servers; use constants/constructors to future-proof.
"lid"/"s.whatsapp.net"are duplicated string literals; prefer usingwacore_binary::jidconstants (e.g., default/hidden server constants) and/or aJid::new(...)-style constructor so futureJidfield additions don’t silently get dropped in manual struct literals.Also applies to: 86-101
120-129: Minor: clean up unused-param handling in defaultget_lid_for_phone.
Prefer naming the arg_phone_user: &strand removinglet _ = phone_user;.wacore/src/store/device.rs (1)
154-172: Init toNoneis fine; consider enforcing a max size at ingestion time (untrusted server data).wacore/binary/src/node.rs (2)
25-36: Borrowing conversion is correct; clarify allocation behavior in docs if perf-sensitive.
NodeContentRef::Nodes(Box<NodeVec>)requires collecting into a newVec, so this is not allocation-free. Consider documenting that this is a “borrowed view” but still allocates containers.
62-75:Node::as_node_ref()looks correct; same note about container allocations.wacore/binary/src/jid.rs (1)
747-916: Tests are solid; add one parity test for “hosted.lid decoded with agent > 0”.You already cover parity for
s.whatsapp.net,lid, andhosted; adding one explicit “agent should be omitted” case forhosted.lidwould close the gap if that decode path ever produces an agent.src/request.rs (1)
165-179: Potential expensive deep-clone on IQ hot path whenArc::try_unwrapfails.If any other task keeps an
Arc<Node>alive,(*arc).clone()may copy the full stanza tree. Consider switching the waiter channel to carryArc<Node>(cheapArc::clone) unless you have a strong reason to require ownership ofNode.src/send.rs (1)
121-126: Don’t silently swallow SKDM-recipient persistence errors.
get_skdm_recipients(...).await.unwrap_or_default()hides DB failures; at least log a warning on error so repeated full SKDM distribution isn’t “mysterious”.src/handlers/message.rs (1)
89-91: Consider adding context to the warning log.When message enqueueing fails, including the chat ID would help with debugging.
if let Err(e) = tx.send(node).await { - warn!("Failed to enqueue message for processing: {e}"); + warn!("Failed to enqueue message for chat {}: {e}", chat_id); }src/lid_pn_cache.rs (1)
211-224: Consider batch insertion forwarm_upto reduce lock contention.Calling
self.add(entry).awaitin a loop acquires and releases bothRwLocks for each entry. For large datasets, this could be slow at startup. A batch approach that acquires each lock once would be more efficient.Example optimization (optional):
pub async fn warm_up(&self, entries: Vec<LidPnEntry>) { let count = entries.len(); let start = std::time::Instant::now(); - for entry in entries { - self.add(entry).await; + // Batch insert to minimize lock acquisitions + { + let mut lid_map = self.lid_to_entry.write().await; + let mut pn_map = self.pn_to_entry.write().await; + + for entry in entries { + lid_map.insert(entry.lid.clone(), entry.clone()); + + let should_update = match pn_map.get(&entry.phone_number) { + Some(existing) => existing.created_at <= entry.created_at, + None => true, + }; + + if should_update { + pn_map.insert(entry.phone_number.clone(), entry); + } + } } log::info!(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
src/appstate_sync.rs(1 hunks)src/client.rs(33 hunks)src/client/context_impl.rs(1 hunks)src/handlers/basic.rs(5 hunks)src/handlers/ib.rs(3 hunks)src/handlers/iq.rs(2 hunks)src/handlers/message.rs(3 hunks)src/handlers/notification.rs(3 hunks)src/handlers/receipt.rs(2 hunks)src/handlers/router.rs(8 hunks)src/handlers/traits.rs(2 hunks)src/handlers/unimplemented.rs(2 hunks)src/handshake.rs(2 hunks)src/lib.rs(1 hunks)src/lid_pn_cache.rs(1 hunks)src/message.rs(8 hunks)src/pair.rs(6 hunks)src/pdo.rs(2 hunks)src/receipt.rs(2 hunks)src/request.rs(2 hunks)src/retry.rs(4 hunks)src/send.rs(7 hunks)src/socket/noise_socket.rs(3 hunks)src/store/signal_adapter.rs(2 hunks)src/usync.rs(1 hunks)storages/sqlite-storage/Cargo.toml(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql(1 hunks)storages/sqlite-storage/src/schema.rs(3 hunks)storages/sqlite-storage/src/sqlite_store.rs(23 hunks)transports/tokio-transport/Cargo.toml(2 hunks)transports/tokio-transport/src/lib.rs(2 hunks)wacore/appstate/src/processor.rs(3 hunks)wacore/binary/Cargo.toml(1 hunks)wacore/binary/src/jid.rs(6 hunks)wacore/binary/src/node.rs(2 hunks)wacore/libsignal/src/protocol/session_cipher.rs(10 hunks)wacore/libsignal/src/protocol/state/session.rs(5 hunks)wacore/src/client/context.rs(6 hunks)wacore/src/pair.rs(2 hunks)wacore/src/send.rs(11 hunks)wacore/src/store/device.rs(2 hunks)wacore/src/store/traits.rs(3 hunks)wacore/src/types/jid.rs(1 hunks)wacore/src/usync.rs(2 hunks)wacore/tests/jid_test.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
wacore/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Files:
wacore/src/usync.rswacore/src/store/device.rswacore/binary/src/node.rswacore/src/pair.rswacore/src/types/jid.rswacore/tests/jid_test.rswacore/appstate/src/processor.rswacore/src/send.rswacore/src/store/traits.rswacore/libsignal/src/protocol/session_cipher.rswacore/binary/src/jid.rswacore/libsignal/src/protocol/state/session.rswacore/src/client/context.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
wacore/src/usync.rswacore/src/store/device.rssrc/usync.rswacore/binary/src/node.rssrc/handlers/iq.rswacore/src/pair.rswacore/src/types/jid.rssrc/request.rssrc/socket/noise_socket.rswacore/tests/jid_test.rssrc/handlers/unimplemented.rswacore/appstate/src/processor.rssrc/handlers/traits.rssrc/handshake.rssrc/appstate_sync.rswacore/src/send.rssrc/lib.rssrc/pdo.rswacore/src/store/traits.rssrc/message.rssrc/retry.rssrc/receipt.rssrc/client/context_impl.rssrc/handlers/ib.rssrc/lid_pn_cache.rssrc/handlers/message.rssrc/store/signal_adapter.rswacore/libsignal/src/protocol/session_cipher.rswacore/binary/src/jid.rswacore/libsignal/src/protocol/state/session.rssrc/handlers/router.rssrc/handlers/receipt.rssrc/send.rssrc/handlers/basic.rssrc/handlers/notification.rswacore/src/client/context.rssrc/pair.rssrc/client.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/usync.rssrc/handlers/iq.rssrc/request.rssrc/socket/noise_socket.rssrc/handlers/unimplemented.rssrc/handlers/traits.rssrc/handshake.rssrc/appstate_sync.rssrc/lib.rssrc/pdo.rssrc/message.rssrc/retry.rssrc/receipt.rssrc/client/context_impl.rssrc/handlers/ib.rssrc/lid_pn_cache.rssrc/handlers/message.rssrc/store/signal_adapter.rssrc/handlers/router.rssrc/handlers/receipt.rssrc/send.rssrc/handlers/basic.rssrc/handlers/notification.rssrc/pair.rssrc/client.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/message.rssrc/send.rssrc/client.rs
🧠 Learnings (6)
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
src/request.rsstorages/sqlite-storage/Cargo.tomlsrc/appstate_sync.rstransports/tokio-transport/Cargo.tomlwacore/binary/Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/socket/noise_socket.rssrc/message.rssrc/retry.rssrc/handlers/message.rssrc/send.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/**/*.rs : All I/O in the main crate must use Tokio; be mindful of race conditions
Applied to files:
src/socket/noise_socket.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
storages/sqlite-storage/Cargo.tomlsrc/handlers/traits.rstransports/tokio-transport/Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rssrc/pair.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to waproto/build.rs : In waproto, use prost in build.rs to compile Protocol Buffers into Rust structs
Applied to files:
wacore/binary/Cargo.toml
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to {src,wacore}/**/*.rs : Use anyhow::Error for functions with multiple failure modes
Applied to files:
src/send.rs
🧬 Code graph analysis (25)
wacore/src/usync.rs (1)
wacore/binary/src/jid.rs (2)
new(305-311)new(362-370)
wacore/src/store/device.rs (1)
storages/sqlite-storage/src/sqlite_store.rs (2)
None(586-586)None(592-592)
src/usync.rs (1)
wacore/src/usync.rs (1)
parse_lid_mappings_from_response(71-109)
src/handlers/iq.rs (2)
wacore/src/xml.rs (1)
DisplayableNode(48-52)src/handlers/traits.rs (1)
handle(26-26)
src/socket/noise_socket.rs (1)
wacore/src/handshake/utils.rs (1)
generate_iv(37-41)
wacore/tests/jid_test.rs (2)
src/jid_utils.rs (1)
server_jid(6-14)wacore/libsignal/src/core/address.rs (1)
name(296-298)
src/handlers/unimplemented.rs (8)
src/handlers/basic.rs (4)
handle(25-28)handle(49-52)handle(73-76)handle(97-105)src/handlers/ib.rs (1)
handle(37-40)src/handlers/iq.rs (1)
handle(31-36)src/handlers/message.rs (1)
handle(35-96)src/handlers/notification.rs (1)
handle(30-33)src/handlers/receipt.rs (1)
handle(28-31)src/handlers/router.rs (1)
handle(106-115)src/handlers/traits.rs (1)
handle(26-26)
storages/sqlite-storage/src/schema.rs (1)
src/store/persistence_manager.rs (1)
device_id(103-105)
src/appstate_sync.rs (2)
storages/sqlite-storage/src/sqlite_store.rs (5)
get_lid_pn_mapping_by_lid(2244-2249)get_lid_pn_mapping_by_phone(2251-2256)put_lid_pn_mapping(2258-2260)get_all_lid_pn_mappings(2262-2264)delete_lid_pn_mapping(2266-2268)wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(103-103)get_lid_pn_mapping_by_phone(106-106)put_lid_pn_mapping(109-109)get_all_lid_pn_mappings(112-112)delete_lid_pn_mapping(115-115)
wacore/src/send.rs (2)
wacore/binary/src/jid.rs (10)
is_hosted(257-259)user(219-219)user(290-292)user(347-349)server(220-220)server(293-295)server(350-352)device(221-221)device(296-298)device(353-355)src/client/context_impl.rs (1)
get_lid_for_phone(32-34)
wacore/src/store/traits.rs (1)
storages/sqlite-storage/src/sqlite_store.rs (5)
get_lid_pn_mapping_by_lid(2244-2249)get_lid_pn_mapping_by_phone(2251-2256)put_lid_pn_mapping(2258-2260)get_all_lid_pn_mappings(2262-2264)delete_lid_pn_mapping(2266-2268)
src/message.rs (5)
wacore/binary/src/attrs.rs (2)
jid(75-78)jid(201-204)src/client.rs (1)
new(169-268)src/store/persistence_manager.rs (3)
new(20-62)device_id(103-105)backend(120-122)wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)src/lid_pn_cache.rs (1)
get_current_lid(156-159)
src/receipt.rs (1)
wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)
src/client/context_impl.rs (2)
wacore/src/client/context.rs (1)
get_lid_for_phone(125-129)wacore/src/send.rs (1)
get_lid_for_phone(917-919)
src/handlers/ib.rs (1)
wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)
src/lid_pn_cache.rs (1)
src/client.rs (1)
new(169-268)
src/store/signal_adapter.rs (1)
wacore/libsignal/src/protocol/state/session.rs (1)
deserialize(575-594)
wacore/libsignal/src/protocol/state/session.rs (3)
wacore/libsignal/src/protocol/ratchet/keys.rs (4)
index(131-133)new(121-123)new(160-162)counter(106-108)wacore/libsignal/src/protocol/sender_keys.rs (3)
new(36-47)new(86-91)new(138-159)wacore/libsignal/src/protocol/state/signed_prekey.rs (1)
new(66-81)
src/handlers/router.rs (1)
wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)
src/handlers/receipt.rs (5)
src/handlers/ib.rs (1)
handle(37-40)src/handlers/iq.rs (1)
handle(31-36)src/handlers/notification.rs (1)
handle(30-33)src/handlers/traits.rs (1)
handle(26-26)src/handlers/unimplemented.rs (1)
handle(49-52)
src/send.rs (2)
src/client.rs (1)
new(169-268)src/store/persistence_manager.rs (1)
new(20-62)
storages/sqlite-storage/src/sqlite_store.rs (1)
wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(103-103)get_lid_pn_mapping_by_phone(106-106)put_lid_pn_mapping(109-109)get_all_lid_pn_mappings(112-112)delete_lid_pn_mapping(115-115)
src/handlers/basic.rs (6)
src/handlers/ib.rs (1)
handle(37-40)src/handlers/iq.rs (1)
handle(31-36)src/handlers/notification.rs (1)
handle(30-33)src/handlers/receipt.rs (1)
handle(28-31)src/handlers/traits.rs (1)
handle(26-26)src/handlers/unimplemented.rs (1)
handle(49-52)
wacore/src/client/context.rs (2)
wacore/binary/src/jid.rs (2)
new(305-311)new(362-370)src/client/context_impl.rs (1)
get_lid_for_phone(32-34)
src/pair.rs (5)
wacore/binary/src/attrs.rs (8)
jid(75-78)jid(201-204)s(86-86)s(107-107)s(138-138)s(213-213)s(235-235)s(266-266)src/bot.rs (1)
client(88-90)wacore/binary/src/node.rs (2)
children(76-81)children(134-139)wacore/src/pair.rs (1)
build_ack_node(68-83)wacore/binary/src/jid.rs (3)
user(219-219)user(290-292)user(347-349)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Build & Test
🔇 Additional comments (73)
wacore/libsignal/src/protocol/state/session.rs (4)
227-247: Good helper extraction; consider documenting the “fail closed” behavior on corrupted chains.
get_receiver_chain_indexnow errors out if any receiver chain is missing/invalid rather than skipping it. That’s likely correct for detecting corrupted session state, but it’s a behavior worth being explicit about since callers may interpret “not found” vs “invalid state” differently.
249-258: LGTM:get_receiver_chainnow returns(Chain, idx)cleanly.
Usingget_receiver_chain_indexkeeps the search logic centralized and avoids repeated clone/search patterns elsewhere.
260-283: Nice: avoids cloning the full chain when only derivingChainKey.
This refactor keeps validation (missing key/index, invalid key length) local and avoids allocating a clonedChainjust to readchain_key.
391-423: Correct in-place removal pattern; borrow discipline looks sound.
Finding the position first and only then mutating the underlyingVecavoids cloning and keeps the mutation localized to the “found” path.storages/sqlite-storage/Cargo.toml (3)
4-4: Verify Rust edition setting.The edition is set to "2024", which is uncommon. While Rust 2024 edition may be stable by now, most projects still use 2021. Please confirm this is intentional and not a typo.
13-17: Diesel feature addition aligns with LID/PN mapping changes.The new "32-column-tables" feature is appropriately scoped to support the additional tables (LidPnMapping, etc.) introduced by the broader LID/PN mapping infrastructure.
Please verify that diesel 2.2.12 supports the "32-column-tables" feature and that it is correctly documented in the diesel changelog or release notes.
24-25: Dependencies verified as secure.Both
log = "0.4.29"andmoka = { version = "0.12", features = ["future"] }are legitimate, available versions with no known security advisories as of December 2025. The moka "future" feature is appropriate for async Rust applications.wacore/appstate/src/processor.rs (2)
154-161: Good:had_no_prior_stateis captured before mutatingstateCapturing
state.versionand the “zero hash” condition before updatingstateavoids TOCTOU issues during later MAC decisions.
185-188: Public signature change: ensure all callers were updated
validate_patch_macs(..., had_no_prior_state)ispub, making this a breaking API change for any external consumers of the wacore crate. Within the repository, the single call site at line 187 has been correctly updated with the newhad_no_prior_stateparameter, but external callers cannot be verified from within this codebase.wacore/libsignal/src/protocol/session_cipher.rs (5)
453-460: Well-designed result type.The
DecryptionResultstruct is clean and well-documented. The explicit flag for tracking session origin makes the control flow clear and enables different identity trust handling paths.
263-263: LGTM: Clean integration of DecryptionResult.The function correctly adapts to the new return type, extracting the plaintext for the final return while maintaining all existing identity and pre-key handling logic.
Also applies to: 286-286
519-521: Correct: Preserve DuplicatedMessage error semantics.The explicit propagation of
DuplicatedMessageerrors ensures that callers can distinguish duplicate messages (which carry chain and counter information) from generic decryption failures. This preserves important error semantics that would otherwise be masked.Also applies to: 584-586
468-468: LGTM: Clear session origin tracking.The function correctly returns
DecryptionResultwith the appropriateused_previous_sessionflag. The flag is set accurately:falsewhen the current session succeeds, andtrueafter promoting a previous session.Also applies to: 514-517, 596-599
317-353: The security concerns raised are not substantiated by the implementation.The code's logic for skipping the identity trust check when a previous session is used is correct and secure. Here's why:
Identity is cryptographically bound to session at creation: Each
SessionStatestores the remote identity key at session establishment time (line 74 instate/session.rs). Once created, a session's identity cannot change. When an old session is promoted to current, its original identity moves with it.Duplicate message protection prevents replays: The code immediately returns
DuplicatedMessageerror (lines 519-520, 584-585) if any message attempts to decrypt with a counter that has already been used in any session. This is checked before attempting promotion, preventing attackers from replaying old ciphertexts to repeatedly promote old sessions.Session promotion is intentional design for out-of-order delivery: When a message decrypts successfully with a previous (archived) session, promoting that session to current is the correct behavior for handling out-of-order message arrival after protocol renegotiation. The identity from that session—which was trusted when the session was originally established—is therefore safe to save without re-checking trust.
The trusting assumption is valid: The comment's rationale is accurate: "When we successfully decrypt with a previous (archived) session, we already had a valid session with that identity—it was trusted when the session was established."
No action is required. The implementation properly handles the out-of-order message delivery scenario and maintains security invariants through session-identity binding and duplicate detection.
wacore/binary/Cargo.toml (1)
23-27: Confirmserdemust be a non-optional build-dependency.
Making build-timeserdeunconditional (Line 25) means it’s pulled in even when the crate’s runtimeserdefeature is off. If build.rs only needs serde for specific feature paths, consider gating the build-dep behind a feature to avoid extra compile/dependency surface.transports/tokio-transport/Cargo.toml (1)
10-14: Add guardrails sodanger-skip-tls-verifycan’t be enabled accidentally.
The naming/comment helps, but please double-check workspace feature unification (e.g., top-level crate features) so this never becomes enabled in production builds unintentionally.Also applies to: 34-42
src/lib.rs (1)
36-38: Public module export looks fine.wacore/src/usync.rs (2)
6-13:UsyncLidMappingshape looks right for persistence.
69-109: Confirm MSRV supports let-chains in this crate (if let Ok(...) = ... && ...).If MSRV is lower than let-chains support, rewrite to nested
if let/match. Otherwise, parsing/filtering behavior looks good (safe fallbacks viacontinue).wacore/tests/jid_test.rs (2)
31-37: Updated server-only JID formatting expectation is consistent and improves coverage.
126-145: Good coverage for WhatsApp-Web-likeProtocolAddressformatting on LID JIDs.wacore/src/store/device.rs (1)
120-124:#[serde(default)]onedge_routing_infois the right compatibility move.src/client/context_impl.rs (1)
31-35: Nice, minimalSendContextResolverintegration (delegates to cache cleanly).src/handlers/receipt.rs (2)
5-6: Import update is consistent with the Arc handler migration.
28-31: Receipt handler now matches the newStanzaHandlerinterface; flow is straightforward.storages/sqlite-storage/src/schema.rs (1)
29-49: Schema additions look consistent (table + allowlist + nullable column).Please double-check the corresponding migration and Diesel model structs reflect:
device.edge_routing_infoasOption<Vec<u8>>lid_pn_mappingprimary key(lid, device_id)Also applies to: 59-68, 112-124
src/handlers/iq.rs (1)
6-8: Arc-based handler update is consistent with the newStanzaHandlerAPI.Also applies to: 31-36
src/pdo.rs (1)
144-146: The&wa::Messageborrow is safe—no latent lifetime issue exists here.
add_recent_messageserializes the message synchronously viaencode_to_vec()and stores only the encoded bytes, never the reference. The wacoreprepare_*functions also take borrowed references and complete synchronously without spawning tasks or enqueuing with the message. No background task retains the borrow past the await boundary.src/handlers/traits.rs (1)
4-4: LGTM! Sound API change for async handler pattern.Switching from
&NodeRef<'_>toArc<Node>eliminates lifetime constraints at async boundaries, enabling handlers to store or forward nodes without cloning. The updated doc comment accurately reflects the new ownership semantics.Also applies to: 20-20, 26-26
src/handlers/router.rs (2)
49-60: LGTM! Router dispatch correctly updated for Arc.The dispatch method properly accepts
Arc<Node>and passes it through to handlers. The tag lookup vianode.tag.as_str()is correct for theHashMap<&'static str, _>key type.
163-170: LGTM! Test scaffolding correctly migrated to Arc.Tests properly construct owned
Nodeinstances wrapped inArc, matching the updated dispatch signature. Both handler-found and handler-not-found paths are covered.Also applies to: 194-201
src/handlers/unimplemented.rs (1)
49-52: LGTM! Consistent migration to Arc.The handler correctly accepts
Arc<Node>and accesses the tag via&node.tag. The control flow remains unchanged (always returnstrueafter handling).src/appstate_sync.rs (1)
520-552: LGTM! Test mock correctly implements new LidPnMappingStore trait.The stub implementation satisfies the trait bounds for
MockBackendin tests. ReturningOk(None)/Ok(vec![])is appropriate since these tests focus on app state sync, not LID/PN mapping functionality.wacore/src/types/jid.rs (2)
67-134: LGTM! Comprehensive test coverage for signal address formatting.Tests cover key scenarios including LID JIDs with/without devices, phone numbers with server mapping, and the edge case of dots in LID user IDs. The protocol address format tests verify the
.0suffix pattern.
36-39: No special handling needed for group JIDs.Group JIDs (
g.us) are used to identify groups in the system, butto_protocol_address()andto_signal_address_string()are only called on individual participant JIDs or LID JIDs—never on group JIDs themselves. Group JIDs are passed as strings toSenderKeyNamefor group identification, not converted to protocol addresses. The current server mapping (which convertss.whatsapp.net→c.usfor phone numbers and leaves other servers likeg.usunchanged) is correct and handles all necessary cases.storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql (1)
18-20:ALTER TABLE ... ADD COLUMN edge_routing_info BLOBlooks fine (nullable / non-breaking).src/send.rs (1)
11-20: Passing&wa::Messagethrough the async send path is fine as long as you only serialize/clone it.Please double-check
Client::add_recent_message(...)and any downstream usage doesn’t retain the reference beyond the call (it should eagerly serialize/clone).src/retry.rs (3)
59-71: Retry now cleanly “take once” from cache; good early-return behavior.
137-156: Passing&original_msgintosend_message_implis a good simplification (no Arc churn).
295-332: Test updates look correct for the new “insert then take” API.src/handlers/notification.rs (1)
30-33: Arc-based handler signature looks consistent with the broader refactor.wacore/src/store/traits.rs (1)
147-176: The concern about breaking no_std compatibility is not applicable here. wacore currently depends unconditionally onasync-traitandtokio, and all existingBackendstore traits (IdentityStore,SessionStore,AppStateKeyStore,AppStateStore,SenderKeyStoreHelper,SenderKeyDistributionStore,DevicePersistence) already use#[async_trait]. AddingLidPnMappingStorefollows the established pattern with no new dependencies or compatibility implications. Existing backends (SqliteStore,MockBackend) already implement this trait without issue.src/message.rs (3)
338-346: Per-sender session locking implementation looks correct.The use of
session_lockscache withget_withto lazily create per-sender mutexes is a good pattern for preventing race conditions during concurrent message processing from the same sender. This aligns with the coding guideline to serialize per-chat operations using appropriate locks.
64-165: LID-based session resolution logic is well-documented and correctly implements WhatsApp Web behavior.The detailed comments explaining WhatsApp Web's
WAWebSignalAddress.toString()behavior provide excellent context. The three-branch logic (LID sender, PN sender with LID mapping, PN sender without mapping) correctly mirrors the expected behavior. The bidirectional cache population (LID→PN and PN→LID) ensures consistency.
2517-2757: Comprehensive test coverage for LID-PN cache integration.The tests thoroughly cover the key scenarios:
- Cache population on message reception with
sender_lid- No cache pollution without
sender_lid- Bidirectional caching for LID senders with
participant_pn- Repeated message handling
- PN messages using cached LID for session lookup
This provides good confidence in the cache behavior.
src/handlers/ib.rs (3)
37-41: Handler signature correctly updated toArc<Node>.The migration from
NodeRef<'_>toArc<Node>aligns with the broader refactor across handlers. Delegating tohandle_ib_implwith a reference (&node) is efficient as it avoids unnecessary cloning.
67-94: Edge routing info handling is well-implemented with proper validation.The implementation:
- Validates presence of
routing_infochild node- Checks for
NodeContent::Bytescontent type- Validates non-empty bytes before storing
- Logs appropriately at each failure point
The async closure in
modify_devicecorrectly captures and movesrouting_bytes.
126-130: Offline sync signaling correctly implemented.Using
Ordering::Relaxedis appropriate here since the atomic flag is coordinated withnotify_waiters()which provides the necessary synchronization for waiters. The pattern correctly mimics WhatsApp Web'sofflineDeliveryEndevent as noted in the comment.src/handlers/message.rs (1)
35-96: Per-chat mailbox pattern correctly prevents race conditions.The two-phase locking approach (enqueue lock → queue access → send) ensures messages are enqueued in arrival order even under concurrent access. This is a good solution for the PreKey message ordering problem described in the comments.
A few observations:
- The channel capacity of 10,000 is generous for backpressure
- Workers are spawned lazily and cleaned up via the cache TTL (5 minutes per the relevant snippet from
src/client.rs)- When the channel receiver is dropped (cache eviction), the worker task will exit gracefully when
recv()returnsNoneAs per coding guidelines, this correctly uses
Client::chat_lockssemantics (nowmessage_queues+message_enqueue_locks) to serialize per-chat operations.src/pair.rs (3)
26-36: Node API migration looks correct.The switch from
attr_parser().get("from")tonode.attrs.get("from").map(|s| s.as_str())correctly accesses the attribute using the new Node API. The comparison againstSERVER_JIDis appropriate.
226-237: Self LID-PN mapping addition is critical for self-messaging.This correctly adds the bot's own LID-to-PN mapping after successful pairing. The guard condition ensures both JID fields are non-empty before caching. Using
LearningSource::Pairingprovides good provenance for debugging.The comment clearly explains why this is needed: when sending DMs to self, the existing LID-based session should be found instead of creating a new PN-based one.
40-46: VerifyPairUtils::build_ack_nodehandles the newNodetype.The call to
PairUtils::build_ack_node(node)now passes&Nodeinstead of the previous type. Based on the relevant snippet fromwacore/src/pair.rs, this function accepts&Node, so the migration is correct.wacore/src/send.rs (5)
113-170: LID session resolution logic is well-designed.The three-phase session resolution:
- Check for existing session under PN address → use PN
- No PN session + LID mapping exists → check for LID session → use LID if found
- No session under either → need prekeys, default to PN
This correctly handles the case where a session was established via a received message with
sender_lid, ensuring replies reuse that session instead of creating a new PN-based one.The separation of
device_jid(for<to>attribute) fromencryption_jid(for actual encryption) is correct - the server expects the original addressing while we use the appropriate session internally.
607-648: LID conversion and hosted device filtering for groups is correct.The logic:
- Converts phone-based device JIDs to LID format for LID groups (lines 607-619)
- Deduplicates after conversion to handle overlapping queries (lines 621-625)
- Filters out hosted devices (device 99, @HosteD server) from SKDM distribution (lines 627-648)
The comment at lines 627-633 clearly explains the filtering rationale:
- Exact sending device is excluded (we have our own sender key)
- Other own devices are kept (they need SKDM to decrypt our messages)
- Hosted devices are excluded (don't participate in group E2EE)
This matches WhatsApp Web's
getFanOutListbehavior as documented in the test comments.
739-742: Addressing mode attribute added for LID groups.Adding
addressing_mode="lid"for LID groups aligns with WhatsApp Web behavior and ensures proper handling by the server.
1036-1163: Comprehensive tests for hosted device identification and filtering.The tests clearly document:
- What hosted devices are (Cloud API / Meta Business API)
- How they're identified (device 99, @HosteD server)
- Why they're filtered from groups (don't use Signal protocol)
- The expected behavior for 1:1 chats vs groups
The
test_hosted_devices_filtered_from_group_skdmtest provides good coverage of the filtering logic with various device types.
1210-1401: LID-PN session mismatch fix tests are thorough.The tests validate the critical scenario that caused the original bug:
test_lid_session_lookup_scenario: Verifies the lookup flowtest_lid_jid_preserves_companion_device_id: Ensures device ID 33 (WhatsApp Web) is preservedtest_lid_lookup_only_for_pn_jids: Confirms LID lookup only applies tos.whatsapp.netJIDsThese tests provide confidence that the fix handles edge cases correctly.
src/handlers/basic.rs (2)
25-28: LGTM!The handler correctly uses the new
Arc<Node>signature and passes a reference toclient.handle_success. This is consistent with the trait definition and other handlers.
97-105: LGTM!The
Arc::try_unwrapwithunwrap_or_elsefallback is the correct pattern for obtaining an ownedNodewhenhandle_ack_responserequires ownership. This avoids unnecessary cloning when theArchas a single reference.src/lid_pn_cache.rs (3)
186-205: Two separate lock acquisitions create a brief inconsistency window.The
add()method releases thelid_to_entrylock before acquiringpn_to_entry. During this window, a concurrent reader could see an inconsistent state (LID present but PN not yet updated). This is mitigated by timestamp-based conflict resolution, so it's not a correctness bug, but worth noting.For the current use case (message processing where exact consistency isn't critical), this is acceptable.
78-89: LGTM!The
LidPnEntrystruct is well-documented with clear field descriptions. The design supports bidirectional lookups and timestamp-based conflict resolution as documented.
44-76: LGTM!The
as_str/parseroundtrip is clean and the fallback toOtherfor unknown strings is a safe default.storages/sqlite-storage/src/sqlite_store.rs (3)
844-879: Background writes trade durability for performance.The cache-first, fire-and-forget DB write pattern means
put_identity_for_devicereturnsOk(())even if the DB write eventually fails. This is intentional for Signal protocol hot paths, and failures are logged. However, a process crash before the DB write completes would lose the identity.This tradeoff is appropriate for the use case. The warning logs at lines 874-875 ensure operators can detect persistent storage issues.
1097-1179: LGTM - good batch optimization for group messaging.The
get_addresses_with_sessionsmethod efficiently combines cache lookups with a single batched DB query for cache misses. The negative caching (storingNonefor non-existent sessions) prevents repeated DB queries.
139-161: LGTM - cache configuration looks reasonable.100k capacity with 1-hour TTL is appropriate for identity/session caches. The moka cache handles eviction automatically.
src/client.rs (8)
545-563: LGTM - critical node inline processing prevents race conditions.Processing
success,failure, andstream:errorinline ensures login state is set before checkingexpected_disconnector spawning other tasks. This prevents subtle races during reconnection.
906-929: Solid guard against duplicate<success>stanzas and reconnect races.The atomic
swaponis_logged_in(line 917) combined withexpected_disconnectcheck (line 910) ensures only the first<success>per connection is processed. Theconnection_generationincrement (line 924) invalidates stale post-login tasks from previous connections.
258-265: Fire-and-forget warm-up is acceptable but consider handling errors more explicitly.The warm-up is spawned without awaiting, so errors are only logged. This is fine since cache warm-up is best-effort and shouldn't block client creation. However, if warm-up consistently fails, callers won't know.
For now this is acceptable since the cache degrades gracefully (just misses).
1520-1527: Good: transport disconnect spawned in background to avoid blocking message loop.Cloning the
Arc<Transport>before spawning ensures the lock isn't held during the async disconnect. This prevents deadlocks if disconnect needs to access other client state.
298-326: LGTM - cache + background persistence pattern.The method correctly:
- Updates in-memory cache synchronously for immediate availability
- Spawns background task for persistence to avoid blocking message processing
- Logs warnings on persistence failures
This matches the pattern used in
sqlite_store.rsfor identity/session writes.
90-109: New concurrency primitives look well-designed.
session_locks: Per-device Signal session locks prevent races when processing messages from the same sender across chatsmessage_queues: Per-chat queues ensure message ordering (critical for PreKey message ordering)message_enqueue_locks: Serialize enqueue operations to prevent race during queue initializationThe 5-minute TTL and 10k capacity are reasonable bounds.
Based on learnings, these align with the guideline to "Use Client::chat_locks to serialize per-chat operations" - these caches provide that serialization.
952-961: Clean macro pattern for generation checking.The
check_generation!macro provides a concise way to bail out of stale post-login tasks. Using a macro avoids repetitive boilerplate while keeping the early-return pattern explicit.
2079-2261: LGTM - comprehensive test coverage for LID-PN cache integration.Tests cover:
- Basic cache operations (insert, lookup)
- Timestamp-based conflict resolution
- Integration with
SendContextResolvertraitThis validates the critical path for LID-PN session reuse.
| -- Remove edge_routing_info column from device table | ||
| -- SQLite doesn't support DROP COLUMN directly in older versions, but newer SQLite (3.35+) does | ||
| -- For compatibility, we create a new table without the column and migrate data | ||
| CREATE TABLE device_backup AS SELECT | ||
| id, lid, pn, registration_id, noise_key, identity_key, signed_pre_key, | ||
| signed_pre_key_id, signed_pre_key_signature, adv_secret_key, account, | ||
| push_name, app_version_primary, app_version_secondary, app_version_tertiary, | ||
| app_version_last_fetched_ms | ||
| FROM device; | ||
| DROP TABLE device; | ||
| ALTER TABLE device_backup RENAME TO device; | ||
|
|
There was a problem hiding this comment.
Down migration is unsafe: CREATE TABLE ... AS SELECT drops constraints/types/indexes.
This rollback will recreate device without PK/FK/NOT NULL/defaults (and any indexes/triggers), which can break the app after a downgrade. Rework to: (1) CREATE TABLE device_backup (...full schema without edge_routing_info...), (2) INSERT INTO device_backup(cols...) SELECT cols... FROM device, (3) drop/rename, and (4) re-create the original indexes/constraints.
🤖 Prompt for AI Agents
In
storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql
around lines 1-12, the current use of "CREATE TABLE device_backup AS SELECT ..."
will drop constraints, types, defaults, PK/FK and indexes; replace this with an
explicit safe migration: (1) create device_backup with the full original device
table schema but without the edge_routing_info column (include PK, NOT NULLs,
defaults, FK clauses, and any triggers), (2) run an INSERT INTO
device_backup(col1, col2, ...) SELECT col1, col2, ... FROM device to copy data
explicitly (list columns in the same order), (3) DROP TABLE device and RENAME
device_backup TO device, and (4) re-create any indexes, triggers and foreign key
constraints that existed on device (if not included in the CREATE), ensuring the
final schema matches the original except for the removed column.
| CREATE TABLE lid_pn_mapping ( | ||
| lid TEXT NOT NULL, -- LID user part (e.g., "100000012345678") | ||
| phone_number TEXT NOT NULL, -- Phone number user part (e.g., "559980000001") | ||
| created_at INTEGER NOT NULL, -- Unix timestamp when mapping was first learned | ||
| learning_source TEXT NOT NULL, -- Source of the mapping (usync, peer_pn_message, etc.) | ||
| updated_at INTEGER NOT NULL, -- Unix timestamp of last update | ||
| device_id INTEGER NOT NULL, -- Device ID for multi-account support | ||
| PRIMARY KEY (lid, device_id), | ||
| FOREIGN KEY(device_id) REFERENCES device(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Index for reverse lookup (phone number -> LID) | ||
| CREATE INDEX idx_lid_pn_mapping_phone ON lid_pn_mapping(phone_number, device_id); |
There was a problem hiding this comment.
Reverse lookup “most recent” isn’t guaranteed without an ordering/index strategy.
idx_lid_pn_mapping_phone (phone_number, device_id) won’t help pick the “most recent” mapping if multiple rows exist per phone. If callers expect “most recent”, consider either enforcing uniqueness or indexing for ORDER BY updated_at DESC:
-CREATE INDEX idx_lid_pn_mapping_phone ON lid_pn_mapping(phone_number, device_id);
+CREATE INDEX idx_lid_pn_mapping_phone_updated
+ ON lid_pn_mapping(phone_number, device_id, updated_at DESC);Also consider whether you want a UNIQUE(phone_number, device_id) constraint (if the mapping is intended to be 1:1 per device).
📝 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.
| CREATE TABLE lid_pn_mapping ( | |
| lid TEXT NOT NULL, -- LID user part (e.g., "100000012345678") | |
| phone_number TEXT NOT NULL, -- Phone number user part (e.g., "559980000001") | |
| created_at INTEGER NOT NULL, -- Unix timestamp when mapping was first learned | |
| learning_source TEXT NOT NULL, -- Source of the mapping (usync, peer_pn_message, etc.) | |
| updated_at INTEGER NOT NULL, -- Unix timestamp of last update | |
| device_id INTEGER NOT NULL, -- Device ID for multi-account support | |
| PRIMARY KEY (lid, device_id), | |
| FOREIGN KEY(device_id) REFERENCES device(id) ON DELETE CASCADE | |
| ); | |
| -- Index for reverse lookup (phone number -> LID) | |
| CREATE INDEX idx_lid_pn_mapping_phone ON lid_pn_mapping(phone_number, device_id); | |
| CREATE TABLE lid_pn_mapping ( | |
| lid TEXT NOT NULL, -- LID user part (e.g., "100000012345678") | |
| phone_number TEXT NOT NULL, -- Phone number user part (e.g., "559980000001") | |
| created_at INTEGER NOT NULL, -- Unix timestamp when mapping was first learned | |
| learning_source TEXT NOT NULL, -- Source of the mapping (usync, peer_pn_message, etc.) | |
| updated_at INTEGER NOT NULL, -- Unix timestamp of last update | |
| device_id INTEGER NOT NULL, -- Device ID for multi-account support | |
| PRIMARY KEY (lid, device_id), | |
| FOREIGN KEY(device_id) REFERENCES device(id) ON DELETE CASCADE | |
| ); | |
| -- Index for reverse lookup (phone number -> LID) | |
| CREATE INDEX idx_lid_pn_mapping_phone_updated | |
| ON lid_pn_mapping(phone_number, device_id, updated_at DESC); |
🤖 Prompt for AI Agents
In
storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql
around lines 4-16, the current index idx_lid_pn_mapping_phone(phone_number,
device_id) does not guarantee efficient selection of the “most recent” mapping
when multiple rows exist per phone; either enforce a 1:1 mapping by adding a
UNIQUE(phone_number, device_id) constraint to the table definition or add an
index that covers ordering by timestamp (e.g., a composite index including
updated_at such as ON (phone_number, device_id, updated_at)) so ORDER BY
updated_at DESC can be satisfied by the index — choose and apply one of these
fixes and update migration text accordingly.
| /// Creates a TLS connector based on feature flags | ||
| fn create_tls_connector() -> Connector { | ||
| // Install rustls crypto provider (only once) | ||
| CRYPTO_PROVIDER_INIT.call_once(|| { | ||
| let _ = rustls::crypto::ring::default_provider().install_default(); | ||
| }); | ||
|
|
||
| #[cfg(feature = "danger-skip-tls-verify")] | ||
| { | ||
| use std::sync::Arc as StdArc; | ||
| use tokio_rustls::TlsConnector; | ||
|
|
||
| warn!("TLS certificate verification is DISABLED - this is insecure!"); | ||
|
|
||
| // Create a custom verifier that accepts any certificate | ||
| #[derive(Debug)] | ||
| struct NoVerifier; | ||
|
|
||
| impl rustls::client::danger::ServerCertVerifier for NoVerifier { | ||
| fn verify_server_cert( | ||
| &self, | ||
| _end_entity: &rustls::pki_types::CertificateDer<'_>, | ||
| _intermediates: &[rustls::pki_types::CertificateDer<'_>], | ||
| _server_name: &rustls::pki_types::ServerName<'_>, | ||
| _ocsp_response: &[u8], | ||
| _now: rustls::pki_types::UnixTime, | ||
| ) -> Result<rustls::client::danger::ServerCertVerified, rustls::Error> { | ||
| Ok(rustls::client::danger::ServerCertVerified::assertion()) | ||
| } | ||
|
|
||
| fn verify_tls12_signature( | ||
| &self, | ||
| _message: &[u8], | ||
| _cert: &rustls::pki_types::CertificateDer<'_>, | ||
| _dss: &rustls::DigitallySignedStruct, | ||
| ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> | ||
| { | ||
| Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) | ||
| } | ||
|
|
||
| fn verify_tls13_signature( | ||
| &self, | ||
| _message: &[u8], | ||
| _cert: &rustls::pki_types::CertificateDer<'_>, | ||
| _dss: &rustls::DigitallySignedStruct, | ||
| ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> | ||
| { | ||
| Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) | ||
| } | ||
|
|
||
| fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> { | ||
| vec![ | ||
| rustls::SignatureScheme::RSA_PKCS1_SHA256, | ||
| rustls::SignatureScheme::RSA_PKCS1_SHA384, | ||
| rustls::SignatureScheme::RSA_PKCS1_SHA512, | ||
| rustls::SignatureScheme::ECDSA_NISTP256_SHA256, | ||
| rustls::SignatureScheme::ECDSA_NISTP384_SHA384, | ||
| rustls::SignatureScheme::ECDSA_NISTP521_SHA512, | ||
| rustls::SignatureScheme::RSA_PSS_SHA256, | ||
| rustls::SignatureScheme::RSA_PSS_SHA384, | ||
| rustls::SignatureScheme::RSA_PSS_SHA512, | ||
| rustls::SignatureScheme::ED25519, | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| let config = rustls::ClientConfig::builder() | ||
| .dangerous() | ||
| .with_custom_certificate_verifier(StdArc::new(NoVerifier)) | ||
| .with_no_client_auth(); | ||
|
|
||
| let tls_connector = TlsConnector::from(StdArc::new(config)); | ||
| Connector::Rustls(tls_connector) | ||
| } | ||
|
|
||
| #[cfg(not(feature = "danger-skip-tls-verify"))] | ||
| { | ||
| use std::sync::Arc as StdArc; | ||
| use tokio_rustls::TlsConnector; | ||
|
|
||
| let mut root_store = rustls::RootCertStore::empty(); | ||
| root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); | ||
|
|
||
| let config = rustls::ClientConfig::builder() | ||
| .with_root_certificates(root_store) | ||
| .with_no_client_auth(); | ||
|
|
||
| let tls_connector = TlsConnector::from(StdArc::new(config)); | ||
| Connector::Rustls(tls_connector) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add additional friction to danger-skip-tls-verify and confirm rustls version compatibility.
The feature-gating provides a first layer of protection, but this accepts any certificate without verification—a high-risk operation. Consider restricting activation further by:
- Adding
cfg(any(test, debug_assertions))in addition to the feature flag, or - Requiring an explicit runtime opt-in (environment variable).
This prevents accidental insecure releases in production builds. Additionally, confirm this implementation compiles and functions correctly with your pinned versions of rustls, tokio-rustls, and webpki-roots. While the ServerCertVerifier trait and RootCertStore APIs are relatively stable, version mismatches can still surface issues. Verify against your specific dependency lock.
🤖 Prompt for AI Agents
transports/tokio-transport/src/lib.rs lines 21-111: the danger-skip-tls-verify
block currently unconditionally enables an insecure verifier by feature flag
alone; restrict activation by either making the compile-time gate stricter
(e.g., require both the feature and test/debug_assertions) or require an
explicit runtime opt-in (check an env var and refuse/emit a loud warning unless
set), and then ensure the custom ServerCertVerifier and RootCertStore usage
matches the exact rustls/tokio-rustls/webpki-roots versions in Cargo.lock
(adjust trait method names and types or imports if needed) so the code compiles
against your pinned dependencies.
| /// | ||
| /// # Arguments | ||
| /// * `patch` - The patch to validate | ||
| /// * `state` - The hash state AFTER applying the patch mutations | ||
| /// * `keys` - The expanded app state keys for MAC computation | ||
| /// * `collection_name` - The collection name | ||
| /// * `had_no_prior_state` - If true, skip ALL MAC validation. This should be true | ||
| /// when processing patches without a prior local state (e.g., first sync without snapshot). | ||
| /// WhatsApp Web handles this case by throwing a retryable error ("empty lthash"), but we | ||
| /// can safely skip validation and process the mutations for usability. The state will be | ||
| /// corrected on the next proper sync with a snapshot. | ||
| pub fn validate_patch_macs( | ||
| patch: &wa::SyncdPatch, | ||
| state: &HashState, | ||
| keys: &ExpandedAppStateKeys, | ||
| collection_name: &str, | ||
| had_no_prior_state: bool, | ||
| ) -> Result<(), AppStateError> { | ||
| // Skip ALL MAC validation if we had no prior state. | ||
| // When we receive patches without a snapshot for a never-synced collection, | ||
| // WhatsApp Web throws a retryable "empty lthash" error. We can't properly validate | ||
| // either the snapshotMac (computed from wrong baseline) or the patchMac (which | ||
| // includes the snapshotMac). Instead, we process the mutations and rely on | ||
| // future syncs with snapshots to correct the state. | ||
| if had_no_prior_state { | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t skip patch_mac validation when had_no_prior_state
You can’t validate snapshot_mac against a missing baseline, but you can still validate patch_mac (it authenticates the patch + its snapshot_mac field). Skipping it reduces integrity even with validate_macs=true (e.g., dropped/reordered mutations won’t be detected at the patch level).
Suggested change (skip only the snapshot-mac check; keep patch-mac check):
pub fn validate_patch_macs(
patch: &wa::SyncdPatch,
state: &HashState,
keys: &ExpandedAppStateKeys,
collection_name: &str,
had_no_prior_state: bool,
) -> Result<(), AppStateError> {
- // Skip ALL MAC validation if we had no prior state.
- // When we receive patches without a snapshot for a never-synced collection,
- // WhatsApp Web throws a retryable "empty lthash" error. We can't properly validate
- // either the snapshotMac (computed from wrong baseline) or the patchMac (which
- // includes the snapshotMac). Instead, we process the mutations and rely on
- // future syncs with snapshots to correct the state.
- if had_no_prior_state {
- return Ok(());
- }
-
- if let Some(snap_mac) = patch.snapshot_mac.as_ref() {
- let computed_snap = state.generate_snapshot_mac(collection_name, &keys.snapshot_mac);
- if computed_snap != *snap_mac {
- return Err(AppStateError::PatchSnapshotMACMismatch);
- }
- }
+ // If we have no baseline state, we can't validate snapshot_mac against our local state,
+ // but we can still validate patch_mac (authenticates the patch contents).
+ if !had_no_prior_state {
+ if let Some(snap_mac) = patch.snapshot_mac.as_ref() {
+ let computed_snap = state.generate_snapshot_mac(collection_name, &keys.snapshot_mac);
+ if computed_snap != *snap_mac {
+ return Err(AppStateError::PatchSnapshotMACMismatch);
+ }
+ }
+ }
if let Some(patch_mac) = patch.patch_mac.as_ref() {
let version = patch.version.as_ref().and_then(|v| v.version).unwrap_or(0);
let computed_patch = generate_patch_mac(patch, collection_name, &keys.patch_mac, version);
if computed_patch != *patch_mac {
return Err(AppStateError::PatchMACMismatch);
}
}
Ok(())
}Also applies to: 271-277
🤖 Prompt for AI Agents
In wacore/appstate/src/processor.rs around lines 236-263 (and also update lines
271-277), don’t return early when had_no_prior_state; instead only skip
snapshot_mac validation but still compute and verify the patch_mac. Replace the
current if had_no_prior_state { return Ok(()) } with a boolean (e.g.,
skip_snapshot_mac_check = had_no_prior_state) and use that flag where
snapshot_mac is validated to bypass just that check; ensure the patch_mac path
always runs (compute expected patch_mac from the patch, state, and keys and
compare it to the provided patch_mac, returning an error on mismatch). Update
the later block at 271-277 to remove any early-exit that skips patch_mac
validation and use the new flag to conditionally skip only the snapshot_mac
verification.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
storages/sqlite-storage/src/sqlite_store.rs (1)
787-839: Fire-and-forget identity/session writes lack graceful shutdown; validate application-level durability guarantees.
put_identity_for_deviceandput_session_for_device(andput_session_for_deviceat 918–968) spawn background DB writes and returnOk(())immediately without awaiting completion. Errors are silently logged. Additionally:
- No
Dropimpl or graceful shutdown mechanism waits for spawned background tasks; pending writes are lost if the process exits before the tasks complete.- The comment "Update cache first to keep the hot path off disk" is misleading—
SqliteStorehas no cache field; the store queries the database directly on every read.PRAGMA synchronous = NORMALmeans writes are not immediately durable to disk; withPRAGMA journal_mode = WAL, durability depends on checkpoint timing.Confirm that the application has explicit shutdown handling to flush pending writes (e.g., draining tokio task joiners or ensuring sufficient delay before process exit) and that callers do not assume writes are persisted immediately after the method returns.
src/client.rs (1)
1646-1650:is_connected()can return false spuriously due totry_lock()contention.
Becausenoise_socketis atokio::sync::Mutex,try_lock()fails if any task is holding it briefly (e.g.,send_node), which can cause post-login tasks to incorrectly treat the client as disconnected.Suggested direction: track connection state via an
AtomicBool(set when noise_socket is set/cleared), or makeis_connectedasync and uselock().await.src/message.rs (1)
362-408: Theunwrap_err()pattern is idiomatic for rand 0.9 but violates error handling guidelines. The patternlet rng = rand::rngs::OsRng; ... &mut rng.unwrap_err()is the documented, correct approach in rand 0.9 to convertTryRngCoretoRngCorefor APIs requiring the infallible trait. However,unwrap_err()will silently panic if OS RNG fails, which violates the guideline to avoid.unwrap()and.expect()outside of truly unrecoverable paths. OS RNG failure in this async context is not necessarily unrecoverable—consider either handling the error explicitly or documenting why panicking is acceptable here.
🧹 Nitpick comments (4)
src/socket/noise_socket.rs (1)
103-109: Consider mutex contention impact with spawn_blocking.The lock is held across the
spawn_blockingcall (lines 103-107) and subsequent.await. While this is necessary for correctness (to maintain counter-to-send ordering), it means large message encryption fully serializes concurrent sends—other tasks block even though the actual CPU work runs on a separate thread.This trade-off is acceptable for correctness, but worth noting if throughput of concurrent large messages becomes a concern in the future.
wacore/src/store/traits.rs (1)
86-118: Good addition; consider derivingPartialEq/EqforLidPnMappingEntryto ease testing and cache comparisons.-#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct LidPnMappingEntry {src/handshake.rs (1)
28-50: Remove redundant length checks + unreachableRoutingInfoTooLargematch arm.
You already pre-checkrouting_info.len() > MAX_EDGE_ROUTING_LEN(Lines 65-73), soErr(HandshakeError::RoutingInfoTooLarge)frombuild_edge_routing_preintrois effectively unreachable (Lines 84-91). Either rely on the helper only, or keep the call-site check and make the helper infallible.- } else { - match build_edge_routing_preintro(routing_info) { + } else { + match build_edge_routing_preintro(routing_info) { Ok(mut header) => { @@ - Err(HandshakeError::RoutingInfoTooLarge) => { - warn!( - target: "Client", - "Routing info unexpectedly exceeds {} bytes; skipping pre-intro", - MAX_EDGE_ROUTING_LEN - ); - wacore_binary::consts::WA_CONN_HEADER.to_vec() - } Err(err) => return Err(err), } }Also applies to: 63-97
src/client.rs (1)
90-110: New cache/lock infrastructure is reasonable; consider reconciling with existing per-chat serialization guidance.
Given the repo guidance to serialize per-chat operations (viaClient::chat_locks), please double-check the newmessage_queues/message_enqueue_locksstrategy doesn’t diverge in a way that can deadlock with other locks (sender/session/chat).Also applies to: 201-213
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
src/client.rs(33 hunks)src/handshake.rs(3 hunks)src/message.rs(9 hunks)src/pair.rs(6 hunks)src/socket/noise_socket.rs(4 hunks)src/usync.rs(2 hunks)storages/sqlite-storage/Cargo.toml(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql(1 hunks)storages/sqlite-storage/src/schema.rs(3 hunks)storages/sqlite-storage/src/sqlite_store.rs(22 hunks)wacore/src/store/traits.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql
- src/usync.rs
- storages/sqlite-storage/Cargo.toml
- storages/sqlite-storage/src/schema.rs
- storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/pair.rssrc/socket/noise_socket.rssrc/handshake.rssrc/message.rssrc/client.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
src/pair.rssrc/socket/noise_socket.rswacore/src/store/traits.rssrc/handshake.rssrc/message.rssrc/client.rs
wacore/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Files:
wacore/src/store/traits.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/message.rssrc/client.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
src/pair.rsstorages/sqlite-storage/src/sqlite_store.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/socket/noise_socket.rssrc/message.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/**/*.rs : All I/O in the main crate must use Tokio; be mindful of race conditions
Applied to files:
src/socket/noise_socket.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
src/socket/noise_socket.rsstorages/sqlite-storage/src/sqlite_store.rssrc/client.rs
🧬 Code graph analysis (2)
wacore/src/store/traits.rs (2)
storages/sqlite-storage/src/sqlite_store.rs (5)
get_lid_pn_mapping_by_lid(2116-2121)get_lid_pn_mapping_by_phone(2123-2128)put_lid_pn_mapping(2130-2132)get_all_lid_pn_mappings(2134-2136)delete_lid_pn_mapping(2138-2140)src/appstate_sync.rs (5)
get_lid_pn_mapping_by_lid(522-527)get_lid_pn_mapping_by_phone(529-534)put_lid_pn_mapping(536-541)get_all_lid_pn_mappings(543-547)delete_lid_pn_mapping(549-551)
storages/sqlite-storage/src/sqlite_store.rs (1)
wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(105-105)get_lid_pn_mapping_by_phone(108-108)put_lid_pn_mapping(111-111)get_all_lid_pn_mappings(114-114)delete_lid_pn_mapping(117-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (12)
src/socket/noise_socket.rs (1)
20-24: Well-designed concurrency control for frame ordering.The mutex addition with clear documentation correctly addresses the race condition where concurrent
encrypt_and_sendcalls could result in frames being sent out-of-order relative to their counter values. Usingtokio::sync::Mutexis appropriate for async code per project guidelines.Also applies to: 35-35
wacore/src/store/traits.rs (2)
86-118: Verifywacoreno_std compatibility for the new serde/async_trait usage (alloc-backedStringfields).This file stays “no Tokio / no DB”, but serde derives +
String/Vecrequire the crate feature set to be correct forno_std + alloc. Please confirmwacorestill compiles in its no_std configuration after this change (feature flags for serde/alloc, etc.). As per coding guidelines, wacore must remain no_std-compatible.
149-178: Backend bound update looks correct; ensure downstream backends implementLidPnMappingStorein multi-account scenarios.Given
DevicePersistencehas explicit*_for_deviceAPIs butLidPnMappingStoredoes not, please sanity-check the intended scoping (single-device-only vs “current device”) for multi-account mode to avoid cross-device mapping leakage.storages/sqlite-storage/src/sqlite_store.rs (4)
55-82: EnablingPRAGMA foreign_keys = ONper connection is the right move.
165-188:with_semaphorepattern looks fine (permits held across blocking work).
213-305:edge_routing_infopersistence wiring looks consistent across save/load/create paths.Please double-check the schema/migrations for
device.edge_routing_info(type/nullability) matchOption<Vec<u8>>end-to-end (Diesel schema + migration).Also applies to: 307-396, 398-500, 688-784, 536-555
1921-2140: LID↔PN mapping helpers +LidPnMappingStoreimpl look consistent (device_id defaults to 1).src/handshake.rs (1)
10-24: Good: panic/DoS risk replaced with typed error + size limit.
This looks like a solid fix vs the priorassert!approach, and theMAX_EDGE_ROUTING_LENguard is clear.src/pair.rs (1)
26-36: Server-JID gate looks correct with new Node attrs access.
The attrs.get(...).map(|s| s.as_str()) pattern reads cleanly and avoids panics.src/message.rs (2)
914-926: DM sender_alt extraction by server type looks correct.
Usingsender_pnfor LID senders andsender_lidfor PN senders is a clean split and should help session resolution.
350-359: Moka single-flight initialization and Jid key choice are sound; however, verify the locking strategy aligns with architecture.Moka 0.12.11 guarantees that
Cache::get_with()coalesces concurrent initialization calls per key into a single execution, preventing redundant mutex creation. TheJidtype derivesHashandEqusing all fields (user, server, agent, device, integrator), so lock granularity matches sender identity precisely.One clarification: AGENTS.md references
Client::chat_locksfor per-chat operations, but the actual per-chat message ordering is handled bymessage_queuesinsrc/handlers/message.rs, whilesession_locksserializes Signal protocol operations per sender/recipient. Confirm this two-level locking pattern (per-chat message queues + per-sender Signal operations) is the intended design.src/client.rs (1)
544-565: Concurrent node processing after sequential decryption is sensible—verify ordering assumptions.
Spawning non-critical stanza handlers can change inter-stanza ordering vs prior behavior; please verify that any handlers relying on ordering are either “critical” or internally serialized (e.g., per-chat/per-sender).
| /// Add a LID-PN mapping to both the in-memory cache and persistent storage. | ||
| /// This is called when we learn about a mapping from messages, usync, etc. | ||
| pub(crate) async fn add_lid_pn_mapping( | ||
| &self, | ||
| lid: &str, | ||
| phone_number: &str, | ||
| source: LearningSource, | ||
| ) -> Result<()> { | ||
| use wacore::store::traits::LidPnMappingEntry; | ||
|
|
||
| // Add to in-memory cache | ||
| let entry = LidPnEntry::new(lid.to_string(), phone_number.to_string(), source); | ||
| self.lid_pn_cache.add(entry.clone()).await; | ||
|
|
||
| // Persist to storage in background (don't block message processing) | ||
| let backend = self.persistence_manager.backend(); | ||
| let storage_entry = LidPnMappingEntry { | ||
| lid: entry.lid, | ||
| phone_number: entry.phone_number, | ||
| created_at: entry.created_at, | ||
| updated_at: entry.created_at, | ||
| learning_source: entry.learning_source.as_str().to_string(), | ||
| }; | ||
|
|
||
| backend | ||
| .put_lid_pn_mapping(&storage_entry) | ||
| .await | ||
| .map_err(|e| anyhow!("persisting LID-PN mapping: {e}"))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Hot-path DB write: add_lid_pn_mapping claims “background” but awaits the write.
This can put SQLite writes on the message/pairing critical path and increase “database is locked” churn. Either update the comment and accept the sync write, or actually spawn/batch persistence (and make failures best-effort).
- // Persist to storage in background (don't block message processing)
+ // Persist to storage (NOTE: this currently awaits; consider spawning if this is hot-path)
let backend = self.persistence_manager.backend();
@@
- backend
- .put_lid_pn_mapping(&storage_entry)
- .await
- .map_err(|e| anyhow!("persisting LID-PN mapping: {e}"))?;
+ backend
+ .put_lid_pn_mapping(&storage_entry)
+ .await
+ .map_err(|e| anyhow!("persisting LID-PN mapping: {e}"))?;(If you want the true “background” behavior, I can sketch a minimal queue-based persister that preserves ordering and de-dupes.)
📝 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.
| /// Add a LID-PN mapping to both the in-memory cache and persistent storage. | |
| /// This is called when we learn about a mapping from messages, usync, etc. | |
| pub(crate) async fn add_lid_pn_mapping( | |
| &self, | |
| lid: &str, | |
| phone_number: &str, | |
| source: LearningSource, | |
| ) -> Result<()> { | |
| use wacore::store::traits::LidPnMappingEntry; | |
| // Add to in-memory cache | |
| let entry = LidPnEntry::new(lid.to_string(), phone_number.to_string(), source); | |
| self.lid_pn_cache.add(entry.clone()).await; | |
| // Persist to storage in background (don't block message processing) | |
| let backend = self.persistence_manager.backend(); | |
| let storage_entry = LidPnMappingEntry { | |
| lid: entry.lid, | |
| phone_number: entry.phone_number, | |
| created_at: entry.created_at, | |
| updated_at: entry.created_at, | |
| learning_source: entry.learning_source.as_str().to_string(), | |
| }; | |
| backend | |
| .put_lid_pn_mapping(&storage_entry) | |
| .await | |
| .map_err(|e| anyhow!("persisting LID-PN mapping: {e}"))?; | |
| Ok(()) | |
| } | |
| /// Add a LID-PN mapping to both the in-memory cache and persistent storage. | |
| /// This is called when we learn about a mapping from messages, usync, etc. | |
| pub(crate) async fn add_lid_pn_mapping( | |
| &self, | |
| lid: &str, | |
| phone_number: &str, | |
| source: LearningSource, | |
| ) -> Result<()> { | |
| use wacore::store::traits::LidPnMappingEntry; | |
| // Add to in-memory cache | |
| let entry = LidPnEntry::new(lid.to_string(), phone_number.to_string(), source); | |
| self.lid_pn_cache.add(entry.clone()).await; | |
| // Persist to storage (NOTE: this currently awaits; consider spawning if this is hot-path) | |
| let backend = self.persistence_manager.backend(); | |
| let storage_entry = LidPnMappingEntry { | |
| lid: entry.lid, | |
| phone_number: entry.phone_number, | |
| created_at: entry.created_at, | |
| updated_at: entry.created_at, | |
| learning_source: entry.learning_source.as_str().to_string(), | |
| }; | |
| backend | |
| .put_lid_pn_mapping(&storage_entry) | |
| .await | |
| .map_err(|e| anyhow!("persisting LID-PN mapping: {e}"))?; | |
| Ok(()) | |
| } |
| // Determine the JID to use for end-to-end decryption. | ||
| // | ||
| // CRITICAL: WhatsApp Web ALWAYS uses LID-based addresses for Signal sessions when | ||
| // a LID mapping is known. This is implemented in WAWebSignalAddress.toString(): | ||
| // | ||
| // var n = o("WAWebWidFactory").asUserWidOrThrow(this.wid); | ||
| // var a = !n.isLid() && n.isUser(); // true if PN | ||
| // var i = a ? o("WAWebApiContact").getCurrentLid(n) : n; // Get LID if PN | ||
| // if (i == null) { | ||
| // return [this.wid.user, t, "@c.us"].join(""); // No LID, use PN | ||
| // } else { | ||
| // return [i.user, t, "@lid"].join(""); // Use LID | ||
| // } | ||
| // | ||
| // This means sessions are stored under the LID address, not the PN address. | ||
| // When we receive a PN-addressed message, we must look up the session using | ||
| // the LID address (if a LID mapping is known) to match WhatsApp Web's behavior. | ||
| let sender_encryption_jid = { | ||
| let sender = &info.source.sender; | ||
| let alt = info.source.sender_alt.as_ref(); | ||
| let pn_server = wacore_binary::jid::DEFAULT_USER_SERVER; | ||
| let lid_server = wacore_binary::jid::HIDDEN_USER_SERVER; | ||
|
|
||
| if sender.server == lid_server { | ||
| if let Some(alt_jid) = alt { | ||
| if alt_jid.server == pn_server { | ||
| alt_jid.clone() | ||
| } else { | ||
| // Alt is another LID variant; stick with the original LID sender. | ||
| sender.clone() | ||
| // Sender is already LID - use it directly for session lookup. | ||
| // Also cache the LID-to-PN mapping if PN alt is available. | ||
| if let Some(alt_jid) = alt | ||
| && alt_jid.server == pn_server | ||
| { | ||
| if let Err(err) = self | ||
| .add_lid_pn_mapping( | ||
| &sender.user, | ||
| &alt_jid.user, | ||
| crate::lid_pn_cache::LearningSource::PeerLidMessage, | ||
| ) | ||
| .await | ||
| { | ||
| warn!( | ||
| "Failed to persist LID-to-PN mapping {} -> {}: {err}", | ||
| sender.user, alt_jid.user | ||
| ); | ||
| } | ||
| } else if info.source.is_from_me { | ||
| // Self-sent LID message without PN alt — try to fall back to our PN identity. | ||
| if let Some(own_pn) = self.get_pn().await { | ||
| log::debug!( | ||
| "Self-sent message from LID {}, using own phone number {}:{} for decryption", | ||
| sender, | ||
| own_pn.user, | ||
| sender.device | ||
| debug!( | ||
| "Cached LID-to-PN mapping: {} -> {}", | ||
| sender.user, alt_jid.user | ||
| ); | ||
| } | ||
| sender.clone() | ||
| } else if sender.server == pn_server { | ||
| // Sender is PN - check if we have a LID mapping. | ||
| // WhatsApp Web uses LID for sessions when available. | ||
|
|
||
| // First, cache/update the mapping if sender_lid attribute is present | ||
| if let Some(alt_jid) = alt | ||
| && alt_jid.server == lid_server | ||
| { | ||
| if let Err(err) = self | ||
| .add_lid_pn_mapping( | ||
| &alt_jid.user, | ||
| &sender.user, | ||
| crate::lid_pn_cache::LearningSource::PeerPnMessage, | ||
| ) | ||
| .await | ||
| { | ||
| warn!( | ||
| "Failed to persist PN-to-LID mapping {} -> {}: {err}", | ||
| sender.user, alt_jid.user | ||
| ); | ||
| Jid { | ||
| user: own_pn.user, | ||
| server: own_pn.server, | ||
| agent: own_pn.agent, | ||
| device: sender.device, | ||
| integrator: own_pn.integrator, | ||
| } | ||
| } else { | ||
| log::warn!("Self-sent message from LID but own phone number not available"); | ||
| sender.clone() | ||
| } | ||
| debug!( | ||
| "Cached PN-to-LID mapping: {} -> {}", | ||
| sender.user, alt_jid.user | ||
| ); | ||
|
|
||
| // Use the LID from the message attribute for session lookup | ||
| let lid_jid = Jid { | ||
| user: alt_jid.user.clone(), | ||
| server: lid_server.to_string(), | ||
| device: sender.device, | ||
| agent: sender.agent, | ||
| integrator: sender.integrator, | ||
| }; | ||
| log::debug!( | ||
| "Using LID {} for session lookup (sender was PN {})", | ||
| lid_jid, | ||
| sender | ||
| ); | ||
| lid_jid | ||
| } else if let Some(lid_user) = self.lid_pn_cache.get_current_lid(&sender.user).await | ||
| { | ||
| // No sender_lid attribute, but we have a cached LID mapping | ||
| let lid_jid = Jid { | ||
| user: lid_user.clone(), | ||
| server: lid_server.to_string(), | ||
| device: sender.device, | ||
| agent: sender.agent, | ||
| integrator: sender.integrator, | ||
| }; | ||
| log::debug!( | ||
| "Using cached LID {} for session lookup (sender was PN {})", | ||
| lid_jid, | ||
| sender | ||
| ); | ||
| lid_jid | ||
| } else { | ||
| // No PN alt provided and not self-sent. Keep the original LID sender. | ||
| // No LID mapping known - use PN address | ||
| log::debug!("No LID mapping for {}, using PN for session lookup", sender); | ||
| sender.clone() | ||
| } | ||
| } else { | ||
| // Sender already uses PN (or another stable server). Never upgrade to LID. | ||
| // Other server type (e.g., bot, hosted) - use as-is | ||
| sender.clone() | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix misleading “Cached mapping” debug logs (they run even on persistence failure).
Both branches log “Cached … mapping” unconditionally after add_lid_pn_mapping even if it returned Err (Lines 93-110, 120-137). Gate the debug log on success.
- if let Err(err) = self
+ match self
.add_lid_pn_mapping(
@@
- .await
- {
- warn!(
- "Failed to persist LID-to-PN mapping {} -> {}: {err}",
- sender.user, alt_jid.user
- );
- }
- debug!(
- "Cached LID-to-PN mapping: {} -> {}",
- sender.user, alt_jid.user
- );
+ .await
+ {
+ Ok(()) => debug!("Cached LID-to-PN mapping: {} -> {}", sender.user, alt_jid.user),
+ Err(err) => warn!("Failed to persist LID-to-PN mapping {} -> {}: {err}", sender.user, alt_jid.user),
+ }🤖 Prompt for AI Agents
In src/message.rs around lines 64 to 177, the debug logs that say "Cached
LID-to-PN mapping" and "Cached PN-to-LID mapping" run even when
add_lid_pn_mapping returned Err; change the flow to only emit those debug logs
on success: capture the Result from add_lid_pn_mapping, if it is Ok(...) then
call debug!(...) to log the cached mapping, otherwise keep the existing
warn!(...) for the Err path and do not log the success message; ensure both
similar branches (the LID branch and the PN branch) follow this pattern so the
debug message is gated on successful persistence.
| // Add the own LID-PN mapping to the cache so that when sending DMs to self, | ||
| // we can find the existing LID-based session instead of creating a new PN-based one. | ||
| // This is critical for self-messaging to work correctly. | ||
| if !jid.user.is_empty() && !lid.user.is_empty() { | ||
| if let Err(err) = client | ||
| .add_lid_pn_mapping(&lid.user, &jid.user, LearningSource::Pairing) | ||
| .await | ||
| { | ||
| warn!( | ||
| "Failed to persist own LID-PN mapping {} <-> {}: {err}", | ||
| lid.user, jid.user | ||
| ); | ||
| } else { | ||
| info!( | ||
| "Added own LID-PN mapping to cache: {} <-> {}", | ||
| lid.user, jid.user | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid logging full phone/LID identifiers (PII) in pairing logs.
The warn!/info! messages currently include lid.user and jid.user verbatim. Consider masking (e.g., last 2–4 digits) or logging only that a mapping was stored.
- warn!(
- "Failed to persist own LID-PN mapping {} <-> {}: {err}",
- lid.user, jid.user
- );
+ warn!("Failed to persist own LID-PN mapping (redacted): {err}");
@@
- info!(
- "Added own LID-PN mapping to cache: {} <-> {}",
- lid.user, jid.user
- );
+ info!("Added own LID-PN mapping to cache (redacted)");📝 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.
| // Add the own LID-PN mapping to the cache so that when sending DMs to self, | |
| // we can find the existing LID-based session instead of creating a new PN-based one. | |
| // This is critical for self-messaging to work correctly. | |
| if !jid.user.is_empty() && !lid.user.is_empty() { | |
| if let Err(err) = client | |
| .add_lid_pn_mapping(&lid.user, &jid.user, LearningSource::Pairing) | |
| .await | |
| { | |
| warn!( | |
| "Failed to persist own LID-PN mapping {} <-> {}: {err}", | |
| lid.user, jid.user | |
| ); | |
| } else { | |
| info!( | |
| "Added own LID-PN mapping to cache: {} <-> {}", | |
| lid.user, jid.user | |
| ); | |
| } | |
| } | |
| // Add the own LID-PN mapping to the cache so that when sending DMs to self, | |
| // we can find the existing LID-based session instead of creating a new PN-based one. | |
| // This is critical for self-messaging to work correctly. | |
| if !jid.user.is_empty() && !lid.user.is_empty() { | |
| if let Err(err) = client | |
| .add_lid_pn_mapping(&lid.user, &jid.user, LearningSource::Pairing) | |
| .await | |
| { | |
| warn!("Failed to persist own LID-PN mapping (redacted): {err}"); | |
| } else { | |
| info!("Added own LID-PN mapping to cache (redacted)"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/pair.rs around lines 226 to 244, the warn! and info! log calls print full
PII (lid.user and jid.user); change them to avoid logging raw phone/LID values
by masking or redacting the identifiers before logging (e.g., derive a safe
string that keeps only the last 2–4 characters or replace with "<redacted>" or a
short hash). Create a small helper or inline logic to produce masked_lid and
masked_jid, use those in both warn! and info! messages, and ensure no other code
paths in this block log the raw values.
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)
Cargo.toml (1)
35-43: Documentdanger-skip-tls-verifyfeature in README to prevent production misuseThe feature has solid code-level guardrails (proper #[cfg] gating, not in defaults, and a runtime
warn!()log), but it's not mentioned in the README. Add a prominent warning in both the main README and the tokio-transport crate README documenting the feature's testing-only purpose and the runtime warning that fires when enabled.Suggested addition to README.md or a "Features" section:
### Danger: TLS Certificate Verification Bypass The `danger-skip-tls-verify` feature disables TLS certificate verification for testing with mock servers only. **Do not use in production.** When enabled, a `WARN` log message is emitted at startup.storages/sqlite-storage/src/sqlite_store.rs (1)
787-839: Update misleading comment about cache behavior and document fire-and-forget durability semanticsThe comment "Update cache first to keep the hot path off disk" is inaccurate—this function has no cache mechanism. The actual behavior is that
put_identity_for_devicereturnsOk(())before the database write completes (fire-and-forget viatokio::spawn), which differs from the synchronous semantics of read operations likeload_identity_for_devicethat usewith_semaphore(...).await. This durability trade-off should be documented clearly to prevent callers from assuming writes are durable on return.Suggested change:
- // Update cache first to keep the hot path off disk + // Fire-and-forget write: returns before DB persistence completesConsider documenting this fire-and-forget semantics at the function level or offering durable variants if callers require guaranteed durability before return.
Also applies to:
put_session_for_device(918-968)
♻️ Duplicate comments (2)
storages/sqlite-storage/src/sqlite_store.rs (2)
89-96: Pool/semaphore mismatch + misleading comments
pool_size = 32butdb_semaphoreis4, while comments imply they “match” / “reduce lock contention” in a way that no longer describes reality.- .max_size(pool_size) // Limit concurrent connections to reduce memory and lock contention + .max_size(pool_size) // Allow up to `pool_size` conns; concurrency is gated by `db_semaphore` - db_semaphore: Arc::new(tokio::sync::Semaphore::new(4)), // Match pool max_size + db_semaphore: Arc::new(tokio::sync::Semaphore::new(4)), // Limit concurrent DB opsAlso applies to: 120-123
1963-2003: LID↔PN persistence: stale comment +created_atoverwritten on update
Comment says “created_at DESC” but the query orders byupdated_at.desc(). Also, upsert updatescreated_at, which breaks “first learned” semantics unless that’s intended.- // Get the most recent mapping for this phone number (by created_at DESC) + // Get the most recent mapping for this phone number (by updated_at DESC) ... .on_conflict((lid_pn_mapping::lid, lid_pn_mapping::device_id)) .do_update() .set(( lid_pn_mapping::phone_number.eq(&phone_number), - lid_pn_mapping::created_at.eq(created_at), lid_pn_mapping::learning_source.eq(&learning_source), lid_pn_mapping::updated_at.eq(now), ))Also applies to: 2005-2051
🧹 Nitpick comments (2)
storages/sqlite-storage/src/sqlite_store.rs (2)
213-305: Semaphore is bypassed on device upserts (can reintroduce lock contention)
save_device_data{,_for_device}runs blocking writes withoutwith_semaphore/db_semaphore, so these can run at full pool concurrency even though other hot paths are gated. Consider routing these throughwith_semaphorefor consistency.Also applies to: 307-396
841-862: Deletes should probably be semaphore-gated too
delete_identity_for_device/delete_session_for_devicedon’t usewith_semaphore, which can still contribute to “database is locked” under load.Also applies to: 970-991
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/socket/noise_socket.rs(3 hunks)storages/sqlite-storage/src/sqlite_store.rs(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/socket/noise_socket.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to waproto/src/whatsapp.proto : Protocol Buffers definitions reside in waproto/src/whatsapp.proto
Applied to files:
Cargo.toml
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to waproto/build.rs : In waproto, use prost in build.rs to compile Protocol Buffers into Rust structs
Applied to files:
Cargo.toml
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rs
🧬 Code graph analysis (1)
storages/sqlite-storage/src/sqlite_store.rs (2)
storages/sqlite-storage/src/device_aware_store.rs (1)
new(21-23)wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(105-105)get_lid_pn_mapping_by_phone(108-108)put_lid_pn_mapping(111-111)get_all_lid_pn_mappings(114-114)delete_lid_pn_mapping(117-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
55-82: Good: enforcePRAGMA foreign_keys = ONper connection
This is the right place to do it (SQLite FK enforcement is per-connection), and you’re propagating failures so broken connections don’t enter the pool.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client.rs (1)
560-635: Unboundedtokio::spawnper stanza risks runaway memory/CPU under floods.
Non-critical nodes are spawned without any concurrency limit/backpressure. A receipt/notification flood can create thousands of tasks and starve the runtime. Consider a bounded work queue /Semaphore/JoinSetwith max in-flight tasks, or route only specific heavy handlers into spawned tasks while processing others inline.src/message.rs (1)
366-412: Fix RNG initialization to avoid creating temporary wrappers on each use.Line 366 should initialize the RNG once as a mutable variable:
let mut rng = rand::rngs::OsRng.unwrap_err();, then pass&mut rngtomessage_decryptat lines 409 and 485, rather than creating new temporary wrappers with&mut rng.unwrap_err()on each call.- let rng = rand::rngs::OsRng; + let mut rng = rand::rngs::OsRng.unwrap_err();Then replace both occurrences of
&mut rng.unwrap_err()with&mut rng:
- Line 409:
&mut rng- Line 485:
&mut rng
♻️ Duplicate comments (5)
storages/sqlite-storage/src/sqlite_store.rs (1)
2042-2042: Stale comment and created_at mutation on update remain unaddressed.Two issues from previous reviews persist:
- Line 2042: Comment says "by created_at DESC" but Line 2053 orders by
updated_at.desc().- Line 2108:
created_atis updated on conflict, contradicting "time of first creation" semantics.Apply this diff to fix both issues:
- // Get the most recent mapping for this phone number (by created_at DESC) + // Get the most recent mapping for this phone number (by updated_at DESC) .on_conflict((lid_pn_mapping::lid, lid_pn_mapping::device_id)) .do_update() .set(( lid_pn_mapping::phone_number.eq(&phone_number), - lid_pn_mapping::created_at.eq(created_at), lid_pn_mapping::learning_source.eq(&learning_source), lid_pn_mapping::updated_at.eq(now), ))If
created_atis intended to track "time of last re-learning", rename it (e.g.,last_learned_at) to avoid confusion.Also applies to: 2053-2053, 2108-2108
src/client.rs (2)
90-116:session_locksmust not be evictable (Signal mutual exclusion can break).
session_locksis a mokaCachewith TTL +max_capacity, so it can evict while an oldArc<Mutex<()>>is still held, allowing a new mutex to be created for the same key and breaking exclusivity. Consider a non-evicting map (e.g.,DashMap<String, Arc<Mutex<()>>>), or implementWeak-based cleanup so “in-use” locks can’t be replaced.Also applies to: 203-216
300-329: LID↔PN persistence isn’t “background”, and cache+storage can diverge on failure.
The comment says “background”, but the code awaits the DB write. Also, you insert into the in-memory cache before persistence; if the write fails, memory now contains a mapping that storage does not. Either (a) update the comment and treat persistence as required (and consider rolling back the in-memory insert on failure), or (b) actually background the write and make it best-effort (log/metric on failure).src/send.rs (1)
31-47: Lock key must be non-evictable + should match the actual encryption target.
- The
session_lockscache eviction hazard still applies here (see prior notes).- You lock on
encryption_jid, but in the peer path you still passtointoprepare_peer_stanza(), which encrypts usingto_jid.to_protocol_address()(so PN vs LID can diverge). That can reintroduce session mismatch / ratchet races. Prefer passingencryption_jidinto the peer encrypt call.- let stanza_to_send: wacore_binary::Node = if peer && !to.is_group() { + let stanza_to_send: wacore_binary::Node = if peer && !to.is_group() { // Peer messages are only valid for individual users, not groups let device_store_arc = self.persistence_manager.get_device_arc().await; let mut store_adapter = SignalProtocolStoreAdapter::new(device_store_arc); wacore::send::prepare_peer_stanza( &mut store_adapter.session_store, &mut store_adapter.identity_store, - to, + encryption_jid, message, request_id, ) .await?src/message.rs (1)
64-176: “Cached mapping” debug logs still fire even when persistence fails (misleading).
This is the same issue called out previously: both mapping branches unconditionallydebug!("Cached ...")after anErrfromadd_lid_pn_mapping(Line 93-110, Line 120-137). Gate the “Cached …” log on success.- if let Err(err) = self - .add_lid_pn_mapping( - &sender.user, - &alt_jid.user, - crate::lid_pn_cache::LearningSource::PeerLidMessage, - ) - .await - { - warn!( - "Failed to persist LID-to-PN mapping {} -> {}: {err}", - sender.user, alt_jid.user - ); - } - debug!( - "Cached LID-to-PN mapping: {} -> {}", - sender.user, alt_jid.user - ); + match self + .add_lid_pn_mapping( + &sender.user, + &alt_jid.user, + crate::lid_pn_cache::LearningSource::PeerLidMessage, + ) + .await + { + Ok(()) => debug!( + "Cached LID-to-PN mapping: {} -> {}", + sender.user, alt_jid.user + ), + Err(err) => warn!( + "Failed to persist LID-to-PN mapping {} -> {}: {err}", + sender.user, alt_jid.user + ), + }- if let Err(err) = self - .add_lid_pn_mapping( - &alt_jid.user, - &sender.user, - crate::lid_pn_cache::LearningSource::PeerPnMessage, - ) - .await - { - warn!( - "Failed to persist PN-to-LID mapping {} -> {}: {err}", - sender.user, alt_jid.user - ); - } - debug!( - "Cached PN-to-LID mapping: {} -> {}", - sender.user, alt_jid.user - ); + match self + .add_lid_pn_mapping( + &alt_jid.user, + &sender.user, + crate::lid_pn_cache::LearningSource::PeerPnMessage, + ) + .await + { + Ok(()) => debug!( + "Cached PN-to-LID mapping: {} -> {}", + sender.user, alt_jid.user + ), + Err(err) => warn!( + "Failed to persist PN-to-LID mapping {} -> {}: {err}", + sender.user, alt_jid.user + ), + }
🧹 Nitpick comments (7)
src/client.rs (3)
259-269: Warm-up task is fire-and-forget; consider test/runtime determinism hooks.
Spawning warm-up duringClient::new()is fine, but it can introduce timing nondeterminism (especially in tests/benchmarks). Consider gating via config/feature flag, or exposing an async “await warm-up” hook used by tests.Also applies to: 272-298
331-369:resolve_encryption_jidis the right direction; avoid extra clones + centralize JID construction.
Minor:lid_user.clone()is unnecessary, and manually rebuildingJidin multiple places is easy to drift. Consider a small helper liketarget.with_server_user(lid_server, lid_user)(or similar) to avoid field copy mistakes.
707-758: Ack/dispatch refactor toArc<Node>looks clean; reduce double-locking for IQ waiters.
contains_key()under lock followed byhandle_iq_response()can lead to extra lock churn and a small TOCTOU window. If feasible, consider a single “try-handle-response-waiter” path that removes/handles atomically.src/send.rs (1)
80-83: Retry cache insertion happens before encryption; verify consumers handle “failed-to-send” entries.
You cache the message bytes before stanza preparation. If encryption fails, you’ll retain a retry entry that may never be valid to retry. If that’s intended, OK; otherwise consider caching only after successful stanza build (or tagging entries with a state).Also applies to: 279-282
src/message.rs (3)
350-363: Per-sender protocol-address lock is a solid race fix; verify lock ordering + bounded growth.
Using the full protocol address string for the lock key should help avoid ratchet counter races (Line 350-363). Two follow-ups:
- Ensure you don’t deadlock with other locks (notably the per-chat serialization guidance for
src/{client,send,message}.rs). Consider documenting the intended lock ordering if both can be held. Based on learnings, useClient::chat_locksto serialize per-chat operations.- Confirm
self.session_lockshas TTL/eviction; otherwise this can grow unbounded in long-running bots.
445-590: UntrustedIdentity path is safer now; one comment is misleading.
Behavior change (clear identity, keep session so libsignal can archive old state) looks right (Line 445-590). Minor: the retry-path comment says “we cleared the session” (Line 515-517) but the code clears identity, not the session.- // The first attempt triggered UntrustedIdentity, we cleared the session, but meanwhile + // The first attempt triggered UntrustedIdentity, we cleared the old identity, but meanwhile
2534-3108: Test coverage for LID↔PN cache population and PN→LID session lookup behavior is strong.
Only suggestion: there’s a lot of repeated node construction + “encryption JID” derivation; consider a small test helper to reduce duplication and make future format changes cheaper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client.rs(33 hunks)src/message.rs(9 hunks)src/send.rs(6 hunks)storages/sqlite-storage/src/sqlite_store.rs(24 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/message.rssrc/send.rssrc/client.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/message.rssrc/send.rssrc/client.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
src/message.rssrc/send.rssrc/client.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
storages/sqlite-storage/src/sqlite_store.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
storages/sqlite-storage/src/sqlite_store.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/message.rssrc/send.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to {src,wacore}/**/*.rs : Use anyhow::Error for functions with multiple failure modes
Applied to files:
src/send.rs
🧬 Code graph analysis (4)
storages/sqlite-storage/src/sqlite_store.rs (1)
wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(105-105)get_lid_pn_mapping_by_phone(108-108)put_lid_pn_mapping(111-111)get_all_lid_pn_mappings(114-114)delete_lid_pn_mapping(117-117)
src/message.rs (2)
wacore/binary/src/jid.rs (6)
user(219-219)user(290-292)user(347-349)device(221-221)device(296-298)device(353-355)src/lid_pn_cache.rs (1)
get_current_lid(156-159)
src/send.rs (1)
wacore/src/send.rs (2)
new(850-856)prepare_peer_stanza(463-502)
src/client.rs (1)
src/lid_pn_cache.rs (4)
with_timestamp(108-120)parse(62-75)get_current_lid(156-159)get_phone_number(164-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (18)
storages/sqlite-storage/src/sqlite_store.rs (9)
8-8: LGTM! Imports and DeviceRow extension look good.The new imports support retry logic (
warn), concurrency primitives (Arc), and LID-PN mapping features. Theedge_routing_infofield extends the device data model as expected.Also applies to: 10-10, 15-15, 17-17, 41-41
65-65: LGTM! PRAGMA settings are appropriate.Doubling
busy_timeoutto 30 seconds and enablingforeign_keysimproves lock contention handling and ensures referential integrity.Also applies to: 77-80
89-93: Pool and semaphore sizing increased for higher concurrency.Pool size raised to 32 with semaphore at 16 to handle higher load. The semaphore effectively limits concurrent DB operations while the pool provides connection availability.
Also applies to: 122-122
231-231: LGTM! Edge routing info propagation is consistent.The
edge_routing_infofield is properly threaded through all device persistence paths (save/load for both single and multi-device modes).Also applies to: 274-274, 294-294, 330-330, 365-365, 385-385, 430-430, 495-495, 553-553, 724-724, 779-779
794-859: LGTM! Identity write with retry logic is well-implemented.The retry mechanism with exponential backoff (up to 5 retries) gracefully handles SQLite lock contention. Semaphore coordination and error logging provide good observability.
889-909: LGTM! Identity load refactored to use with_semaphore helper.Using the
with_semaphorehelper improves consistency and maintainability for read operations.
916-950: LGTM! Session methods improved with retry logic and semaphore coordination.Session operations now use consistent patterns: reads with
with_semaphore, writes with retry logic and exponential backoff. Debug logging aids troubleshooting.Also applies to: 959-1031, 1033-1054
1063-1108: LGTM! Batch session checking with proper SQLite parameter limit handling.The method chunks addresses into groups of 900, staying well under SQLite's ~999 bind parameter limit. Returns a
HashSetfor efficient membership checking in group messaging scenarios.
2182-2209: LGTM! LidPnMappingStore trait implementation follows established patterns.The trait delegates to device-specific methods with
device_id = 1, consistent with other single-device trait implementations in this file.src/client.rs (3)
658-706: Sequential decrypt + ownedNodereturn looks correct; watch CPU cost of decrypt/decompress.
This keeps Noise counter ordering correct. Ifdecrypt_frame/decompress ever becomes materially CPU-heavy, considerspawn_blockingfor decompress/unmarshal (Tokio guideline), but current structure is reasonable.
949-1065: Offline-sync gating needs strict “set flag then notify” ordering.
The waiting logic is fine if the offline stanza handler doesoffline_sync_completed.store(true, …)beforeoffline_sync_notifier.notify_waiters(). If notify happens first, the waiter can miss it and stall until timeout. Please verify handler ordering.
1546-1571: 515 handling: proactive transport disconnect is a good resilience tweak.
Cloning the transport outside the lock and disconnecting in a spawned task is the right shape to avoid blocking the recv loop.src/send.rs (2)
11-20: Signature change to&wa::Messageis a nice win (less Arc churn).
Call-site update insend_message()looks correct.Also applies to: 22-30
53-66: Peer retry behavior: confirm whether peer messages should be cached too.
Peer path doesn’t calladd_recent_message(), while DM/group paths do. If peer messages can also go through retry flows, you likely want consistent caching (or an explicit comment why not).src/message.rs (4)
6-6: Good logging import split (debug + warn).
579-586: Good: retry receipt on UntrustedIdentity retry failure.
919-930: DM sender_alt extraction by sender type is clear and matches the intent.
1648-1705: Nice coverage for LID protocol address formatting; keep it aligned to the actual implementation.
These assertions will be brittle ifto_protocol_address()formatting changes; if that API is from an external crate/workspace dependency, confirm version pinning and behavior.
…SQLite semaphore handling
0ab77b3 to
f2fd83c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/libsignal/src/protocol/session_cipher.rs (1)
13-17: Critical: Thread-local storage breaksno_stdcompatibility.The
thread_local!macro requires the standard library and violates the coding guideline that wacore must remainno_std-compatible. This will break compilation inno_stdenvironments.Consider alternatives such as:
- Using a mutex-protected static buffer pool
- Passing buffers as parameters through the call stack
- Using stack-allocated arrays with const generics for smaller buffers
As per coding guidelines, wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases.
wacore/src/pair.rs (1)
134-154: Restore HMAC verification: outer container integrity check is currently disabled.The commented-out
mac.verify_slice(hmac_bytes)check (lines 147–153) bypasses verification of the outerAdvSignedDeviceIdentityHmaccontainer. While the inner account signature is still verified, it covers a different scope (inner_details_bytes) and assumes the outer container'sdetails_bytesfield is trustworthy. Without HMAC verification, a modifieddetails_bytesat the outer level is not detected, creating a security gap.Uncomment the verification block and rename
_hmac_bytestohmac_bytesto fix this.- let _hmac_bytes = hmac_container + let hmac_bytes = hmac_container .hmac .as_deref() .ok_or_else(|| PairCryptoError { code: 500, text: "internal-error", source: anyhow::anyhow!("HMAC container missing hmac"), })?; if is_hosted_account { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // if mac.verify_slice(hmac_bytes).is_err() { - // return Err(PairCryptoError { - // code: 401, - // text: "hmac-mismatch", - // source: anyhow::anyhow!("HMAC mismatch"), - // }); - // } + if mac.verify_slice(hmac_bytes).is_err() { + return Err(PairCryptoError { + code: 401, + text: "hmac-mismatch", + source: anyhow::anyhow!("HMAC mismatch"), + }); + }
♻️ Duplicate comments (8)
wacore/appstate/src/processor.rs (1)
254-262: Don't skippatch_macvalidation whenhad_no_prior_state.This implementation still skips ALL MAC validation (including
patch_mac) whenhad_no_prior_stateis true, which directly contradicts the previous review feedback. The earlier comment explicitly stated:"You can't validate
snapshot_macagainst a missing baseline, but you can still validatepatch_mac(it authenticates the patch + itssnapshot_macfield). Skipping it reduces integrity even withvalidate_macs=true."The
patch_macauthenticates the patch contents and doesn't depend on having a prior state baseline—only thesnapshot_maccheck requires the baseline.Apply this diff to skip only
snapshot_macvalidation while preservingpatch_macvalidation:- // Skip ALL MAC validation if we had no prior state. - // When we receive patches without a snapshot for a never-synced collection, - // WhatsApp Web throws a retryable "empty lthash" error. We can't properly validate - // either the snapshotMac (computed from wrong baseline) or the patchMac (which - // includes the snapshotMac). Instead, we process the mutations and rely on - // future syncs with snapshots to correct the state. - if had_no_prior_state { - return Ok(()); - } - - if let Some(snap_mac) = patch.snapshot_mac.as_ref() { - let computed_snap = state.generate_snapshot_mac(collection_name, &keys.snapshot_mac); - if computed_snap != *snap_mac { - return Err(AppStateError::PatchSnapshotMACMismatch); - } - } + // If we have no baseline state, we can't validate snapshot_mac against our local state, + // but we can still validate patch_mac (authenticates the patch contents). + if !had_no_prior_state { + if let Some(snap_mac) = patch.snapshot_mac.as_ref() { + let computed_snap = state.generate_snapshot_mac(collection_name, &keys.snapshot_mac); + if computed_snap != *snap_mac { + return Err(AppStateError::PatchSnapshotMACMismatch); + } + } + }transports/tokio-transport/src/lib.rs (1)
21-111: TLS connector centralization is well-structured.The
create_tls_connector()function cleanly separates TLS configuration from transport creation. The feature-gateddanger-skip-tls-verifypath with warning log is appropriate.Note: A previous review already flagged concerns about adding additional friction to the
danger-skip-tls-verifyfeature (e.g., restricting to test/debug builds or requiring runtime opt-in).src/message.rs (1)
93-110: Fix misleading "Cached mapping" debug logs.The
debug!()logs on lines 106-109 and 133-136 run unconditionally afteradd_lid_pn_mapping, even when persistence fails. This was flagged in a previous review.Apply this fix to both branches:
- if let Err(err) = self + match self .add_lid_pn_mapping( &sender.user, &alt_jid.user, crate::lid_pn_cache::LearningSource::PeerLidMessage, ) .await { - warn!( - "Failed to persist LID-to-PN mapping {} -> {}: {err}", - sender.user, alt_jid.user - ); + Ok(()) => debug!( + "Cached LID-to-PN mapping: {} -> {}", + sender.user, alt_jid.user + ), + Err(err) => warn!( + "Failed to persist LID-to-PN mapping {} -> {}: {err}", + sender.user, alt_jid.user + ), } - debug!( - "Cached LID-to-PN mapping: {} -> {}", - sender.user, alt_jid.user - );Also applies to: 120-137
src/pair.rs (1)
234-243: PII (phone numbers/LIDs) logged in pairing flow.The
warn!andinfo!messages includelid.userandjid.userverbatim, which are sensitive identifiers. Consider masking or redacting these values in logs.src/send.rs (1)
40-46: Session lock cache eviction race condition already flagged.This concern was identified in a previous review: using a moka
Cachewith TTL andmax_capacityforsession_lockscan evict entries whileArcclones are still held, allowing new mutexes to be created and breaking Signal session mutual exclusion.storages/sqlite-storage/src/sqlite_store.rs (2)
2104-2110:created_atupdate on conflict still present.A previous review noted that updating
created_atin the conflict clause contradicts "first learned" semantics. Ifcreated_atshould represent when the mapping was first learned, remove it from the update set:.do_update() .set(( lid_pn_mapping::phone_number.eq(&phone_number), - lid_pn_mapping::created_at.eq(created_at), lid_pn_mapping::learning_source.eq(&learning_source), lid_pn_mapping::updated_at.eq(now), ))If the current behavior (updating
created_at) is intentional, consider renaming the field or updating documentation to clarify its semantics.
2042-2053: Stale comment: says "created_at DESC" but orders byupdated_at.The comment on line 2042 states "by created_at DESC" but the actual query orders by
updated_at.desc(). Update the comment to match the implementation:- // Get the most recent mapping for this phone number (by created_at DESC) + // Get the most recent mapping for this phone number (by updated_at DESC)src/client.rs (1)
300-329: Hot-path DB write:add_lid_pn_mappingclaims "background" but awaits the write.The comment on line 314 states "Persist to storage in background (don't block message processing)", but the implementation awaits the database write (lines 324-327), which blocks the caller. This can add SQLite write latency to the message processing path and increase "database is locked" contention.
Either update the comment to reflect the synchronous behavior, or spawn the persistence as a true background task with best-effort error handling.
🧹 Nitpick comments (8)
wacore/src/client/context.rs (1)
7-19: Consider usingJid::newconstructor for cleaner code.The manual Jid construction could be simplified by using the
Jid::newconstructor. Additionally, consider extracting the hardcoded"lid"server name as a constant for better maintainability.Apply this diff to use the constructor:
fn build_pn_to_lid_map(lid_to_pn_map: &HashMap<String, Jid>) -> HashMap<String, Jid> { lid_to_pn_map .iter() .map(|(lid_user, phone_jid)| { - let lid_jid = Jid { - user: lid_user.clone(), - server: "lid".to_string(), - ..Default::default() - }; + let lid_jid = Jid::new(lid_user, "lid"); (phone_jid.user.clone(), lid_jid) }) .collect() }For the hardcoded server name, consider adding at module level:
const LID_SERVER: &str = "lid"; const PHONE_SERVER: &str = "s.whatsapp.net";wacore/binary/src/jid.rs (1)
177-259: Consider foldingHOSTED_LID_SERVERinto existing classification helpers (is_ad, fallback known servers).
Right nowHOSTED_LID_SERVERexists andis_hosted()checks it, but:
is_ad()doesn’t include it (Line 224-229), which may cause inconsistent behavior for@hosted.lidAD-style JIDs.- The fallback
known_serverslist inFromStrdoesn’t include it (Line 404-416), so server-only"hosted.lid"would be rejected while"s.whatsapp.net"is allowed.pub const HOSTED_SERVER: &str = "hosted"; pub const HOSTED_LID_SERVER: &str = "hosted.lid";fn is_ad(&self) -> bool { self.device() > 0 && (self.server() == DEFAULT_USER_SERVER || self.server() == HIDDEN_USER_SERVER - || self.server() == HOSTED_SERVER) + || self.server() == HOSTED_SERVER + || self.server() == HOSTED_LID_SERVER) }let known_servers = [ DEFAULT_USER_SERVER, GROUP_SERVER, LEGACY_USER_SERVER, BROADCAST_SERVER, HIDDEN_USER_SERVER, NEWSLETTER_SERVER, HOSTED_SERVER, + HOSTED_LID_SERVER, MESSENGER_SERVER, INTEROP_SERVER, BOT_SERVER, STATUS_BROADCAST_USER, ];src/handlers/notification.rs (2)
52-62:versionis computed but never used.The
versionvariable is extracted from attributes but only logged indirectly via the{version}format string. If this is intentional (placeholder for future sync scheduling), consider adding a TODO comment. If sync scheduling should use this value, the implementation is incomplete.let name = collection_node.attrs().string("name"); let mut attrs = collection_node.attrs(); let version = attrs.optional_u64("version").unwrap_or(0); info!( target: "Client/AppState", "scheduling sync for collection '{name}' from version {version}." ); + // TODO: Actually schedule the sync using name and version
30-34: Consider passing&Arc<Node>to preserve cheap cloning.
handle_notification_impltakes&Node, sonode.clone()on lines 75 and 83 performs a deep clone of the Node structure. If you pass&Arc<Node>instead, event dispatch can clone the Arc (cheap) rather than the Node (expensive).-async fn handle_notification_impl(client: &Arc<Client>, node: &Node) { +async fn handle_notification_impl(client: &Arc<Client>, node: &Arc<Node>) {src/handlers/ib.rs (1)
46-60: Minor: Redundant string conversion.
timestampis converted fromOption<&str>→Option<String>→Option<&str>viaas_deref(). This works but is slightly inefficient.let mut attrs = child.attrs(); let dirty_type = attrs.string("type"); - let timestamp = attrs.optional_string("timestamp").map(|s| s.to_string()); + let timestamp = attrs.optional_string("timestamp"); ... if let Err(e) = client_clone - .clean_dirty_bits(&dirty_type, timestamp.as_deref()) + .clean_dirty_bits(&dirty_type, timestamp) .awaitNote: This requires
timestampto be moved into the spawned task or cloned asStringifclean_dirty_bitsneeds ownership.src/handshake.rs (1)
84-91: Unreachable match arm due to prior length check.The
Err(HandshakeError::RoutingInfoTooLarge)arm is unreachable because line 65 already validatesrouting_info.len() > MAX_EDGE_ROUTING_LENand returns early. Consider simplifying withunwrap()(safe here due to prior check) or keeping as defensive code with a comment.match build_edge_routing_preintro(routing_info) { Ok(mut header) => { debug!( target: "Client", "Sending edge routing pre-intro ({} bytes) for optimized reconnection", routing_info.len() ); header.extend_from_slice(&wacore_binary::consts::WA_CONN_HEADER); header } - Err(HandshakeError::RoutingInfoTooLarge) => { - warn!( - target: "Client", - "Routing info unexpectedly exceeds {} bytes; skipping pre-intro", - MAX_EDGE_ROUTING_LEN - ); - wacore_binary::consts::WA_CONN_HEADER.to_vec() - } - Err(err) => return Err(err), + // SAFETY: Length already validated above; RoutingInfoTooLarge is the only error variant + Err(_) => unreachable!("Length validated at line 65"), }Alternatively, keep the current code as defensive programming against future changes to
build_edge_routing_preintro.src/lid_pn_cache.rs (1)
211-224: Consider batch insertion for warm_up optimization.Each
add()call acquires two write locks. For large caches, this could be optimized to acquire locks once and batch insert:pub async fn warm_up(&self, entries: Vec<LidPnEntry>) { let count = entries.len(); let start = std::time::Instant::now(); // Batch insert with single lock acquisition per map { let mut lid_map = self.lid_to_entry.write().await; let mut pn_map = self.pn_to_entry.write().await; for entry in entries { lid_map.insert(entry.lid.clone(), entry.clone()); let should_update = match pn_map.get(&entry.phone_number) { Some(existing) => existing.created_at <= entry.created_at, None => true, }; if should_update { pn_map.insert(entry.phone_number.clone(), entry); } } } log::info!("LID-PN cache warmed up with {} entries in {:?}", count, start.elapsed()); }The timing log will help identify if this becomes a bottleneck in practice.
src/client.rs (1)
588-607: Good critical node handling with inline processing.Processing critical nodes (
success,failure,stream:error) inline ensures state consistency before spawning concurrent tasks. The comment correctly explains thatis_logged_inmust be set before checkingexpected_disconnector spawning other tasks.Optional: Consider extracting the critical node check to a helper function:
fn is_critical_node(tag: &str) -> bool { matches!(tag, "success" | "failure" | "stream:error") }Then use:
if is_critical_node(node.tag.as_str())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.toml(1 hunks)src/appstate_sync.rs(1 hunks)src/client.rs(33 hunks)src/client/context_impl.rs(1 hunks)src/handlers/basic.rs(5 hunks)src/handlers/ib.rs(3 hunks)src/handlers/iq.rs(2 hunks)src/handlers/message.rs(3 hunks)src/handlers/notification.rs(3 hunks)src/handlers/receipt.rs(2 hunks)src/handlers/router.rs(8 hunks)src/handlers/traits.rs(2 hunks)src/handlers/unimplemented.rs(2 hunks)src/handshake.rs(3 hunks)src/lib.rs(1 hunks)src/lid_pn_cache.rs(1 hunks)src/message.rs(9 hunks)src/pair.rs(6 hunks)src/pdo.rs(2 hunks)src/receipt.rs(2 hunks)src/request.rs(2 hunks)src/retry.rs(4 hunks)src/send.rs(6 hunks)src/socket/noise_socket.rs(3 hunks)src/store/signal_adapter.rs(2 hunks)src/usync.rs(2 hunks)storages/sqlite-storage/Cargo.toml(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql(1 hunks)storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql(1 hunks)storages/sqlite-storage/src/schema.rs(3 hunks)storages/sqlite-storage/src/sqlite_store.rs(24 hunks)transports/tokio-transport/Cargo.toml(2 hunks)transports/tokio-transport/src/lib.rs(2 hunks)wacore/appstate/src/processor.rs(3 hunks)wacore/binary/Cargo.toml(1 hunks)wacore/binary/src/jid.rs(6 hunks)wacore/binary/src/node.rs(2 hunks)wacore/libsignal/src/protocol/session_cipher.rs(10 hunks)wacore/libsignal/src/protocol/state/session.rs(5 hunks)wacore/src/client/context.rs(6 hunks)wacore/src/pair.rs(2 hunks)wacore/src/send.rs(11 hunks)wacore/src/store/device.rs(2 hunks)wacore/src/store/traits.rs(3 hunks)wacore/src/types/jid.rs(1 hunks)wacore/src/usync.rs(2 hunks)wacore/tests/jid_test.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- src/pdo.rs
- src/lib.rs
- src/client/context_impl.rs
- storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/down.sql
- wacore/binary/Cargo.toml
- wacore/src/store/device.rs
- src/handlers/traits.rs
- Cargo.toml
- transports/tokio-transport/Cargo.toml
- src/handlers/iq.rs
- src/handlers/unimplemented.rs
- storages/sqlite-storage/src/schema.rs
- wacore/tests/jid_test.rs
- storages/sqlite-storage/migrations/2025-12-11-000000_add_lid_pn_mapping/up.sql
- src/receipt.rs
- wacore/src/usync.rs
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/handlers/receipt.rssrc/handlers/router.rssrc/retry.rssrc/usync.rssrc/request.rssrc/handlers/ib.rssrc/handlers/notification.rssrc/socket/noise_socket.rssrc/handlers/message.rssrc/send.rssrc/handshake.rssrc/message.rssrc/lid_pn_cache.rssrc/pair.rssrc/appstate_sync.rssrc/handlers/basic.rssrc/client.rssrc/store/signal_adapter.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
src/handlers/receipt.rswacore/binary/src/jid.rssrc/handlers/router.rssrc/retry.rssrc/usync.rswacore/appstate/src/processor.rssrc/request.rssrc/handlers/ib.rswacore/binary/src/node.rswacore/src/pair.rswacore/src/types/jid.rssrc/handlers/notification.rssrc/socket/noise_socket.rssrc/handlers/message.rswacore/src/store/traits.rssrc/send.rswacore/src/send.rssrc/handshake.rswacore/libsignal/src/protocol/session_cipher.rswacore/src/client/context.rssrc/message.rssrc/lid_pn_cache.rssrc/pair.rssrc/appstate_sync.rssrc/handlers/basic.rswacore/libsignal/src/protocol/state/session.rssrc/client.rssrc/store/signal_adapter.rs
wacore/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Files:
wacore/binary/src/jid.rswacore/appstate/src/processor.rswacore/binary/src/node.rswacore/src/pair.rswacore/src/types/jid.rswacore/src/store/traits.rswacore/src/send.rswacore/libsignal/src/protocol/session_cipher.rswacore/src/client/context.rswacore/libsignal/src/protocol/state/session.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/send.rssrc/message.rssrc/client.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
storages/sqlite-storage/Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rssrc/pair.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
storages/sqlite-storage/Cargo.tomlsrc/request.rswacore/src/pair.rssrc/socket/noise_socket.rsstorages/sqlite-storage/src/sqlite_store.rssrc/appstate_sync.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/retry.rssrc/usync.rssrc/socket/noise_socket.rssrc/handlers/message.rssrc/send.rssrc/message.rssrc/client.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to waproto/src/whatsapp.proto : Protocol Buffers definitions reside in waproto/src/whatsapp.proto
Applied to files:
wacore/src/types/jid.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/**/*.rs : All I/O in the main crate must use Tokio; be mindful of race conditions
Applied to files:
src/socket/noise_socket.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to {src,wacore}/**/*.rs : Use anyhow::Error for functions with multiple failure modes
Applied to files:
src/send.rs
🧬 Code graph analysis (17)
src/handlers/router.rs (1)
wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)
src/usync.rs (1)
wacore/src/usync.rs (1)
parse_lid_mappings_from_response(71-109)
src/handlers/ib.rs (1)
wacore/binary/src/node.rs (2)
attrs(83-85)attrs(181-185)
wacore/src/types/jid.rs (2)
wacore/binary/src/jid.rs (15)
device(221-221)device(296-298)device(353-355)new(305-311)new(362-370)server(220-220)server(293-295)server(350-352)user(219-219)user(290-292)user(347-349)from_str(385-485)from(213-215)from(557-559)from(563-565)wacore/libsignal/src/core/address.rs (1)
name(296-298)
src/handlers/notification.rs (4)
wacore/binary/src/attrs.rs (2)
jid(75-78)jid(201-204)src/handlers/traits.rs (1)
handle(26-26)wacore/binary/src/node.rs (4)
children(76-81)children(134-139)attrs(83-85)attrs(181-185)wacore/binary/src/builder.rs (2)
children(36-39)attrs(24-34)
wacore/src/store/traits.rs (2)
storages/sqlite-storage/src/sqlite_store.rs (5)
get_lid_pn_mapping_by_lid(2184-2189)get_lid_pn_mapping_by_phone(2191-2196)put_lid_pn_mapping(2198-2200)get_all_lid_pn_mappings(2202-2204)delete_lid_pn_mapping(2206-2208)src/appstate_sync.rs (5)
get_lid_pn_mapping_by_lid(522-527)get_lid_pn_mapping_by_phone(529-534)put_lid_pn_mapping(536-541)get_all_lid_pn_mappings(543-547)delete_lid_pn_mapping(549-551)
src/send.rs (1)
wacore/src/send.rs (2)
new(850-856)prepare_peer_stanza(463-502)
wacore/libsignal/src/protocol/session_cipher.rs (7)
wacore/libsignal/src/protocol/protocol.rs (2)
ciphertext(456-458)counter(126-128)wacore/libsignal/src/protocol/ratchet/params.rs (2)
their_identity_key(76-78)their_identity_key(155-157)wacore/libsignal/src/protocol/identity_key.rs (2)
public_key(39-41)public_key(115-117)wacore/libsignal/src/protocol/state/prekey.rs (1)
public_key(72-79)wacore/libsignal/src/core/curve.rs (2)
public_key(237-245)public_key_bytes(93-97)wacore/libsignal/src/protocol/ratchet/keys.rs (5)
counter(106-108)mac_key(96-98)key(126-128)key(164-166)index(131-133)wacore/libsignal/src/protocol/state/session.rs (1)
root_key(159-165)
wacore/src/client/context.rs (3)
wacore/binary/src/jid.rs (2)
new(305-311)new(362-370)wacore/src/send.rs (2)
new(850-856)get_lid_for_phone(917-919)src/client/context_impl.rs (1)
get_lid_for_phone(32-34)
src/lid_pn_cache.rs (1)
src/client.rs (1)
new(171-270)
storages/sqlite-storage/src/sqlite_store.rs (1)
wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(105-105)get_lid_pn_mapping_by_phone(108-108)put_lid_pn_mapping(111-111)get_all_lid_pn_mappings(114-114)delete_lid_pn_mapping(117-117)
src/pair.rs (5)
src/client.rs (1)
handle_iq(1659-1686)wacore/binary/src/node.rs (2)
children(76-81)children(134-139)wacore/binary/src/builder.rs (2)
children(36-39)bytes(41-44)wacore/src/pair.rs (1)
build_ack_node(68-83)wacore/binary/src/jid.rs (3)
user(219-219)user(290-292)user(347-349)
src/appstate_sync.rs (2)
storages/sqlite-storage/src/sqlite_store.rs (7)
get_lid_pn_mapping_by_lid(2184-2189)None(547-547)None(553-553)get_lid_pn_mapping_by_phone(2191-2196)put_lid_pn_mapping(2198-2200)get_all_lid_pn_mappings(2202-2204)delete_lid_pn_mapping(2206-2208)wacore/src/store/traits.rs (5)
get_lid_pn_mapping_by_lid(105-105)get_lid_pn_mapping_by_phone(108-108)put_lid_pn_mapping(111-111)get_all_lid_pn_mappings(114-114)delete_lid_pn_mapping(117-117)
src/handlers/basic.rs (6)
src/handlers/ib.rs (1)
handle(37-40)src/handlers/iq.rs (1)
handle(31-36)src/handlers/notification.rs (1)
handle(30-33)src/handlers/receipt.rs (1)
handle(28-31)src/handlers/traits.rs (1)
handle(26-26)src/handlers/unimplemented.rs (1)
handle(49-52)
wacore/libsignal/src/protocol/state/session.rs (2)
wacore/libsignal/src/protocol/protocol.rs (2)
chain_key(572-574)counter(126-128)wacore/libsignal/src/protocol/ratchet/keys.rs (4)
index(131-133)new(121-123)new(160-162)counter(106-108)
src/client.rs (1)
src/lid_pn_cache.rs (5)
with_timestamp(108-120)parse(62-75)as_str(46-59)get_current_lid(156-159)get_phone_number(164-167)
src/store/signal_adapter.rs (1)
wacore/libsignal/src/protocol/state/session.rs (1)
deserialize(575-594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (67)
wacore/appstate/src/processor.rs (1)
154-160: LGTM: Correct detection of empty initial state.The logic correctly identifies when there's no prior state by checking both
version == 0and an all-zeros hash. This is the proper baseline for determining MAC validation behavior.wacore/src/client/context.rs (4)
29-31: LGTM! Proper maintenance of bidirectional mapping.The
pn_to_lid_mapfield is correctly initialized in all constructors and properly synchronized withlid_to_pn_mapupdates. The reverse mapping will improve lookup performance for phone-to-LID conversions.Also applies to: 45-45, 55-56, 61-61, 67-67
81-84: LGTM! Clean reverse lookup implementation.The method provides symmetric functionality to
phone_jid_for_lid_userand enables efficient phone-to-LID lookups using the maintained reverse map.
86-101: LGTM! Correct device JID conversion logic.The method properly handles the conversion from phone-based to LID-based device JIDs while preserving the device-specific fields (
device,agent,integrator). The fallback to return the original JID when no mapping exists is appropriate and well-documented.
119-129: LGTM! Well-designed trait extension for LID lookup.The default implementation provides backward compatibility while allowing implementations to override with LID-aware session resolution. The async signature appropriately supports cache or database lookups, as seen in the
context_impl.rsimplementation that delegates tolid_pn_cache.src/socket/noise_socket.rs (6)
5-5: Correct use oftokio::sync::Mutex.Using
tokio::sync::Mutexis appropriate here since the lock will be held across.awaitpoints (bothspawn_blockingandtransport.send). Astd::sync::Mutexwould cause issues when held across async boundaries.
20-24: Clear documentation and correct mutex pattern.The inline documentation effectively explains the ordering requirement. Using
Mutex<()>is idiomatic when synchronization is needed without protecting shared data.
44-46: LGTM!The mutex acquisition at the start ensures the entire operation (counter increment → encryption → framing → send) is serialized, preventing out-of-order frame delivery.
82-96: Good use ofspawn_blockingfor CPU-bound encryption.Offloading large message encryption to the blocking thread pool follows the coding guidelines. The Arc pattern correctly handles ownership transfer into the blocking task.
Note: The mutex remains held during the blocking task, which is intentional for ordering. This could cause contention under high concurrent load with large messages, but that's the necessary tradeoff for correctness.
141-189: Test covers buffer reuse semantics.The test validates that buffers are returned with preserved capacity and cleared contents. Uses
.unwrap()appropriately in test context per coding guidelines.Consider adding a concurrency test in the future to verify the mutex ordering guarantees under parallel
encrypt_and_sendcalls.
127-133: Read path is architecturally single-threaded; concurrent decryption is not a concern.The
decrypt_framemethod is called sequentially from a singlewhileloop inclient.rs(line 588), and the comment explicitly states "Decrypt the frame synchronously (required for noise protocol counter ordering)". Node processing is spawned concurrently after decryption, not during. The synchronous nature ofdecrypt_frameand single-threaded read loop make the counter ordering safe without additional synchronization. The asymmetry withencrypt_and_send(which usessend_mutexto serialize concurrent writes) is intentional: writes can arrive from multiple async tasks, reads arrive from a single event loop.wacore/libsignal/src/protocol/session_cipher.rs (4)
453-460: LGTM!The
DecryptionResultstruct is well-designed with clear documentation explaining the purpose of theused_previous_sessionflag.
263-286: LGTM!The changes correctly adapt to the new
DecryptionResultreturn type. Theused_previous_sessionflag is appropriately unused here since PreKey messages establish new sessions rather than using previous ones.
519-521: LGTM: DuplicatedMessage errors now propagate immediately.The change to return
DuplicatedMessageerrors immediately (rather than collecting them and trying other sessions) is correct. When a duplicate is detected on a specific chain, it's definitive—no other session will change that outcome.Also applies to: 584-586
317-353: No action required – this implementation correctly follows Signal Protocol specifications.The code properly handles archived sessions per the Signal Protocol Sesame session-management specification. When a message is successfully decrypted with an archived (previous) session, that session is promoted to active. Since the archived session was established when the identity was trusted, and the Double Ratchet algorithm provides forward secrecy for those messages independent of future identity changes, skipping the trust check for archived sessions is correct and aligned with the protocol's design.
Signal's identity verification (safety numbers) is intended to detect key changes and authenticate future messages, not retroactively invalidate past messages from sessions that were trusted at establishment time. The implementation's distinction between archived and current sessions follows this model properly.
storages/sqlite-storage/Cargo.toml (1)
13-24: LGTM!The addition of
32-column-tablesDiesel feature is appropriate for supporting wider schema requirements (likely the newlid_pn_mappingtables), and thelogdependency enables standard logging support for the storage backend.wacore/binary/src/node.rs (2)
25-36: LGTM!The
as_content_refmethod correctly converts ownedNodeContentvariants to their borrowedNodeContentRefcounterparts, properly borrowing primitives and recursively mapping nested nodes.
62-74: LGTM!The
as_node_refmethod provides a clean borrow-based conversion from ownedNodetoNodeRef, correctly borrowing the tag, converting attrs entries, and boxing the content reference. This maintains wacore's no_std compatibility.transports/tokio-transport/src/lib.rs (1)
209-209: Verify the 100x increase in event channel capacity.The channel capacity increased from 100 to 10,000. This significantly increases memory footprint per transport instance. Ensure this is intentional and necessary for the expected message throughput, and consider documenting the rationale.
src/request.rs (1)
167-182: LGTM! Clean migration to Arc-based ownership.The
handle_iq_responsemethod correctly transitions toArc<Node>ownership:
- Uses
Arc::try_unwrapto avoid cloning when there are no other references- Falls back to cloning the inner
Nodeonly when necessary- The waiter lookup and send logic remains correct
This aligns with the broader PR-wide shift to Arc-based node handling for the per-chat worker queue architecture.
wacore/libsignal/src/protocol/state/session.rs (3)
227-247: LGTM! Efficient index-based lookup for receiver chains.The new
get_receiver_chain_indexhelper enables index-based access patterns throughout the session state, reducing unnecessary cloning when only the index or in-place mutation is needed.
391-423: LGTM! Efficient in-place message key removal.The refactored
get_message_keysnow:
- Finds the message key position without cloning the chain
- Only mutates when a matching key is found
- Uses direct index access for the removal
This avoids cloning the entire chain just to find and remove a single message key.
430-432: Theexpect()calls are safe and the invariant holds across all call sites.Both
set_message_keys(line 430-432) andset_receiver_chain_key(line 449-451) safely call.expect()onget_receiver_chain_index(). The only call sites for these methods are inget_or_create_message_key(), which is always preceded by a call toget_or_create_chain_key(). This function guarantees that the receiver chain for the given sender already exists in state—either by retrieving an existing chain or by creating and adding a new one before returning. Therefore, when these methods attempt to look up the chain index, it is guaranteed to exist.wacore/src/types/jid.rs (3)
15-42: LGTM! WhatsApp Web-compatible Signal address formatting.The
to_signal_address_stringimplementation correctly:
- Includes device suffix only when
device != 0- Maps
s.whatsapp.nettoc.usto match WhatsApp Web's internal format- Is well-documented with the corresponding JavaScript implementation for reference
44-64: LGTM! Protocol address generation matches WhatsApp Web's createSignalLikeAddress.The
to_protocol_addressimplementation correctly:
- Encodes the device in the name portion via
to_signal_address_string- Uses
device_id = 0for the ProtocolAddress (matching WA Web behavior)- Results in the
{SignalAddress}.0format expected by the protocol
67-134: LGTM! Comprehensive test coverage.Tests thoroughly cover:
- LID JIDs with and without device numbers
- LID JIDs with dots in the user portion
- Phone number JIDs with server mapping
- Protocol address string formatting consistency
src/store/signal_adapter.rs (2)
44-53: LGTM! Simplified adapter without in-memory session cache.The removal of the Moka session cache simplifies the implementation. This aligns with the PR's architectural changes introducing centralized caching via
lid_pn_cachemodule. SQLite's built-in caching should provide adequate performance for session lookups.
56-93: The review comment references methods (with_cache_capacity,invalidate_session,clear_session_cache) that do not exist in the current codebase. A comprehensive search of the entire repository found no definitions or usages of these methods. TheSessionAdapterstruct insrc/store/signal_adapter.rscontains only the trait implementations shown in the provided snippet and no cache management APIs. This breaking change claim cannot be verified against the actual codebase.Likely an incorrect or invalid review comment.
src/message.rs (4)
350-362: LGTM! Per-sender session locking prevents race conditions.The session locking correctly:
- Uses the full Signal protocol address string as the lock key
- Matches the
SignalProtocolStoreAdapter's per-session semantics- Prevents ratchet counter races when processing concurrent messages from the same sender
This aligns with the coding guideline to serialize per-chat operations. Based on learnings, this is the correct approach.
445-586: LGTM! Improved UntrustedIdentity handling preserves in-flight messages.The updated handling correctly:
- Clears only the old identity (not the session)
- Allows the session to be archived when the new PreKeySignalMessage is processed
- Enables decryption of any in-flight messages encrypted with the old session
- Spawns retry receipts when retry fails for various error conditions
The documentation clearly explains the recovery flow and rationale.
919-930: LGTM! Bidirectional sender_alt extraction.The updated logic correctly extracts alternate JID based on sender type:
- LID sender → look for
sender_pnattribute- PN sender → look for
sender_lidattributeThis enables proper session resolution regardless of which identity format the message uses.
2533-3109: Excellent test coverage for LID-PN cache and session behavior.The new tests comprehensively cover:
- Cache population on receiving messages with sender_lid
- Bidirectional LID↔PN mappings
- PN messages using LID for session lookup when mapping is known
- Fallback to PN when no LID mapping exists
- Cache handling for repeated messages and group contexts
src/usync.rs (1)
3-3: Nice: persistence failure is handled (no silent drop).
Warn + continue is the right tradeoff for “best-effort learning” here. (This also addresses the earlier review note.)Also applies to: 48-69
src/handlers/receipt.rs (1)
28-30: Arc migration looks consistent.
Signature + call site update matches the new handler routing flow.src/handlers/router.rs (2)
49-60: Dispatch signature update toArc<Node>is clean.
Usingnode.tag.as_str()for lookup and passing the same Arc into the handler keeps ownership simple and avoids extra cloning.
155-218: Tests updated appropriately for owned Node construction.
The test setup (IndexMap attrs + NodeContent) matches the new API and validates routing behavior.wacore/binary/src/jid.rs (1)
488-527: Server-only JID display change is clear and well-covered by tests.
The “no leading @” behavior is explicit and validated.Also applies to: 659-666
src/appstate_sync.rs (1)
520-552: Good test scaffolding to satisfy newBackend: LidPnMappingStorebound.
Benign defaults are appropriate for these unit tests.src/retry.rs (2)
59-71: Retry flow simplification looks good (early-return on cache miss, no Arc churn).
The updated control flow is easier to follow and matches the updatedsend_message_implsignature.Also applies to: 137-155
317-332: Tests correctly validate take-once semantics after API change.
First take returns the message; second take returns None.wacore/src/store/traits.rs (1)
86-118: LID↔phone mapping store API is well-shaped (includesupdated_at+ integrates into Backend).
This provides the needed metadata for “most recent by phone” lookups and makes the capability uniformly available viaBackend.Also applies to: 149-178
src/handlers/message.rs (1)
35-43: LGTM on the ordering guarantee approach.The double-lock pattern (enqueue_mutex before queue creation) correctly ensures FIFO ordering even when the queue is created lazily. The comment documentation clearly explains the concurrency reasoning. Based on learnings, this aligns with the per-chat serialization requirement.
src/handlers/ib.rs (2)
127-131: LGTM on offline sync completion signaling.The atomic store with
Ordering::Relaxedfollowed bynotify_waiters()correctly signals completion to waiting post-login tasks. TheNotifyensures proper synchronization for tasks that callnotified().await.
71-85: This comment is incorrect. Themodify_device()call is part of a fire-and-forget pattern where in-memory state is modified immediately, and a background saver task handles persistence asynchronously. Errors during persistence are logged via the background task:if let Err(e) = self.save_to_disk().await { error!("Error saving device state in background: {e}"); }. The design is intentional and appropriate for this use case.Likely an incorrect or invalid review comment.
src/pair.rs (2)
26-36: LGTM on Node API migration.The migration from
NodeReftoNodeis clean. Theattrs.get("from").map(|s| s.as_str())pattern correctly handles the optional attribute comparison againstSERVER_JID.
161-175: Good defensive parsing withparser.finish()check.The pattern of using
optional_jid()with a subsequentfinish()check properly handles attribute parsing errors while still providing default values. This is a solid approach for handling potentially malformed pair-success messages.src/handshake.rs (2)
63-97: LGTM on edge routing validation and fallback.The validation at line 65 correctly prevents oversized routing info before calling
build_edge_routing_preintro. The fallback toWA_CONN_HEADERensures graceful degradation. The past review concerns aboutassert!panics have been properly addressed.
31-50: LGTM on pre-intro encoding.The 3-byte big-endian length encoding is correct:
(len >> 16)for high byte,(len >> 8)for middle byte,lenfor low byte. The capacity pre-allocation avoids reallocations.src/handlers/basic.rs (1)
25-28: Consistent Arc migration across all handlers.The signature changes align with the updated
StanzaHandlertrait. TheAckHandlercorrectly usesArc::try_unwrapto attempt zero-copy ownership transfer, falling back to clone when other references exist—this is the idiomatic pattern when an owned value is required.Also applies to: 49-52, 73-76, 97-105
src/send.rs (2)
22-25: Clean migration from Arcwa::Message to borrowed reference.Passing the message by reference is appropriate since it's read-only during the send operation. This reduces unnecessary Arc overhead throughout the sending path.
53-54: Good defensive check for peer message path.Adding
!to.is_group()ensures peer stanzas are only used for individual recipients, preventing incorrect path selection.wacore/src/send.rs (4)
113-170: Well-designed LID-aware session resolution.The logic correctly handles the scenario where a session was established under a LID address (from receiving a message with
sender_lid) but the reply is addressed to a phone number. The mapping preserves the originaldevice_jidfor the XMLtoattribute while using the LID session for encryption.
290-333: Correct separation of encryption and addressing JIDs.The code properly uses the LID-mapped JID for Signal encryption while preserving the original phone number JID for the XML
toattribute. Theunwrap_or(device_jid)is defensive since all devices should be in the map.
1036-1163: Excellent test coverage for LID-PN session handling and hosted device filtering.The tests comprehensively cover:
- Hosted device identification (
is_hosted()behavior)- SKDM distribution filtering logic
- LID session lookup scenarios
- Device ID preservation in LID JID construction
The detailed documentation in test comments (explaining what hosted devices are and why they're filtered) adds valuable context.
Also applies to: 1210-1401
607-648: Correct LID conversion and hosted device filtering for group SKDM.The implementation correctly:
- Converts phone-based JIDs to LID format for LID-addressed groups
- Deduplicates after conversion to handle users appearing under both formats
- Filters hosted/Cloud API devices (device ID 99 or
@hosted/@hosted.lidserver) which don't participate in group E2EEThe
is_hosted()method is properly defined in theJidExttrait and matches the filtering requirements. This matches WhatsApp Web'sgetFanOutListbehavior.src/lid_pn_cache.rs (3)
18-76: Clean serialization pattern for LearningSource.The
as_str()/parse()pattern is appropriate for database storage. Defaulting unknown strings toOtherprovides forward compatibility when new sources are added.
186-205: Correct timestamp-based conflict resolution for PN mappings.The logic correctly implements "newest LID wins" for phone number lookups while preserving all LID entries. The sequential locking pattern is acceptable for the expected low-frequency write operations.
254-437: Comprehensive test coverage for cache operations.Tests cover basic CRUD, timestamp-based conflict resolution (both directions), warm-up, clearing, and serialization roundtrip. Good coverage of the core functionality.
storages/sqlite-storage/src/sqlite_store.rs (5)
65-80: Good PRAGMA configuration for SQLite reliability.
busy_timeout = 30000(30s) is appropriate for WAL mode under concurrent loadforeign_keys = ONensures referential integrity for the newlid_pn_mappingtableBoth correctly applied per-connection.
89-93: Reasonable pool and semaphore configuration.Pool size 32 with semaphore 16 allows connection reuse while limiting concurrent operations. The updated comment accurately describes the intent.
Also applies to: 120-123
787-859: Robust retry logic for identity and session writes.The exponential backoff (10-160ms over 5 retries) is appropriate for SQLite lock contention. The
warn!logging helps diagnose contention issues in production.Also applies to: 952-1031
1063-1108: SQLite bind parameter limit properly handled with chunking.Chunking by 900 stays well under SQLite's typical 999 variable limit. This addresses the concern from a previous review.
41-41: Consistentedge_routing_infopropagation across device operations.The new field is correctly handled across all device CRUD paths: save, load, and create operations for both single-device and multi-device modes.
Also applies to: 231-231, 274-274, 294-294, 330-330, 365-365, 385-385, 430-430, 495-495, 553-553, 724-724, 779-779
src/client.rs (4)
90-111: Excellent concurrency design for per-device and per-chat synchronization.The three-layer locking strategy effectively prevents race conditions:
session_locks: Prevents Signal protocol races when multiple messages from the same sender arrive concurrently across different chatsmessage_queues: Ensures sequential processing within each chat (critical for PreKey messages)message_enqueue_locks: Serializes queue creation to prevent initialization racesThe 5-minute TTL is reasonable and allows idle resources to be reclaimed while
message_enqueue_locksensures safe queue recreation when needed.Based on learnings, this implements the
Client::chat_lockspattern mentioned in the coding guidelines for serializing per-chat operations.Also applies to: 203-215
949-1004: Excellent generation guard pattern to prevent stale task races.The connection generation counter and
check_generation!macro effectively handle reconnection races, particularly during the 515 pairing flow. When a new connection is established, any stale post-login tasks from the previous connection are properly cancelled before performing network operations.This prevents race conditions where:
- Old connection tasks might try to send on a closed socket
- Duplicate passive tasks run after reconnection
- App state sync from old connection interferes with new connection
The macro makes the check consistent and readable across the post-login sequence.
Also applies to: 1037-1037, 1057-1057, 1069-1069, 1075-1075, 1090-1090, 1097-1097, 1138-1138, 1162-1162, 1177-1177
1042-1064: Correct implementation of offline sync flow matching WhatsApp Web.The post-login sequence correctly follows WhatsApp Web's behavior:
- Send active IQ to exit passive mode (line 1038)
- Server responds with
<ib><offline count="X"/></ib>- Wait for offline sync completion before proceeding with passive tasks (prekey upload, presence)
- Timeout (5s) handles edge cases where server doesn't send offline ib
The generation check after the wait (line 1057) properly handles connection replacement during the wait period.
2123-2310: Comprehensive test coverage for LID-PN cache functionality.The tests validate the critical aspects of the LID-PN cache:
- Basic bidirectional mappings (phone↔LID lookup)
- Timestamp-based conflict resolution (newer mapping wins for phone→LID lookup)
- Integration with
SendContextResolvertrait (used by wacore during message encryption)This ensures the cache correctly prevents LID-PN session mismatches when sending replies to messages received with sender_lid attributes.
| log::debug!( | ||
| "{remote_address} derived message_keys: mac_key={}", | ||
| hex::encode(message_keys.mac_key()) | ||
| ); |
There was a problem hiding this comment.
Caution: Debug logs expose cryptographic key material.
The new debug logs output sensitive cryptographic keys (MAC keys, root keys, ephemeral keys) in hexadecimal. While these are debug-level logs typically disabled in production, if debug logging is inadvertently enabled in production environments, this could leak sensitive cryptographic material to log files.
Consider:
- Adding a compile-time feature flag to disable cryptographic material logging in release builds
- Documenting that debug logging must never be enabled in production
- Using log filtering to redact these specific keys even at debug level
Also applies to: 755-760, 763-768
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/prekeys.rs (3)
113-125: Remove or complete the unused key lookup logic.This loop serves no purpose in its current state:
found_countis initialized but never incremented- The condition on line 115 will never be met
- Line 123 breaks immediately when a key is found
The comment acknowledges this is incomplete. Either implement proper key reuse logic or remove this dead code to avoid unnecessary iterations.
Apply this diff to remove the dead code:
- // Check if we have existing unuploaded keys by trying IDs sequentially - // We'll check a reasonable range to find existing keys - let found_count = 0; - for id in 1..=1000u32 { - if found_count >= WANTED_PRE_KEY_COUNT { - break; - } - - if let Ok(Some(_record)) = backend.load_prekey(id).await { - // Check if this key was already uploaded by seeing if it exists on server - // For simplicity, assume unuploaded keys have a specific pattern or we track separately - // For now, we'll use existing keys if available but generate new ones with sequential IDs - break; // We'll generate new ones with better tracking - } - } -
128-137: Critical performance issue: Loop may iterate millions of times.This loop can iterate up to 16,777,215 times with an async backend call per iteration, causing severe performance degradation when many pre-keys exist. Additionally, the logic is incorrect—it breaks at the first gap (line 135) rather than finding the highest existing ID.
Consider maintaining a high-water mark in the device store or using a more efficient backend query:
- // Step 2: Generate new keys with sequential IDs to avoid collisions - let mut highest_existing_id = 0u32; - - // Find the highest existing pre-key ID to start from - for id in 1..=16777215u32 { - if backend.contains_prekey(id).await.unwrap_or(false) { - highest_existing_id = id; - } else { - break; // Found first gap - } - } - - let start_id = highest_existing_id + 1; + // Step 2: Generate new keys with sequential IDs to avoid collisions + // TODO: Implement backend.get_highest_prekey_id() or maintain a counter in device store + let start_id = { + let device_guard = device_store.read().await; + device_guard.next_prekey_id.unwrap_or(1) + };Alternatively, if you must scan for gaps, limit the search range to a reasonable window (e.g., last 1000 IDs).
153-153: Critical: Code will not compile—OsRng.unwrap_err()is invalid.
OsRngis a struct, not aResult. Calling.unwrap_err()on it will cause a compilation error. Based on therand_coreAPI, you should pass&mut OsRngdirectly to the key generation function.Apply this diff:
- let key_pair = KeyPair::generate(&mut OsRng.unwrap_err()); + let key_pair = KeyPair::generate(&mut OsRng);src/socket/error.rs (1)
18-30: Public enum variant addition is a breaking change—call it out / mitigate match breakage.
EncryptSendErrorKindispub; addingChannelClosedcan break downstream exhaustivematches. If you want to reduce future breakage, consider making the enum#[non_exhaustive](likely requires a major bump anyway) and documenting the expectation that callers include a wildcard arm.
♻️ Duplicate comments (4)
storages/sqlite-storage/src/sqlite_store.rs (2)
2099-2115:created_atoverwritten on conflict: semantic ambiguity.Line 2112 updates
created_atduring conflict resolution, which contradicts "first created" semantics. Ifcreated_atshould reflect the original insertion time, remove it from the conflict-update path:.on_conflict((lid_pn_mapping::lid, lid_pn_mapping::device_id)) .do_update() .set(( lid_pn_mapping::phone_number.eq(&phone_number), - lid_pn_mapping::created_at.eq(created_at), lid_pn_mapping::learning_source.eq(&learning_source), lid_pn_mapping::updated_at.eq(now), ))If you intend
created_atto reflect "time this (lid, phone) pair was (re)learned," rename it or document that behavior to avoid confusion.
2046-2057: Comment mismatch: says "created_at DESC" but query orders by "updated_at DESC".Line 2046's comment is outdated.
- // Get the most recent mapping for this phone number (by created_at DESC) + // Get the most recent mapping for this phone number (by updated_at DESC)src/send.rs (1)
37-51: Re-verifysession_lockscannot “recreate” locks via eviction (prior critical issue).
This still uses.session_locks.get_with(... Arc<Mutex<()>> ...); if the backing map is an evicting cache, eviction while clones exist can allow a new mutex to be created later, breaking session mutual exclusion. This looks similar to a previously-addressed review thread—please confirm it’s still fixed at current head.#!/bin/bash set -euo pipefail echo "=== Locate Client::session_locks definition / type ===" rg -n --type rust 'session_locks\s*:' -C 3 echo "=== Locate session_locks builder/config (TTL/capacity/eviction) ===" rg -n --type rust 'session_locks.*Cache::builder|Cache::builder\(\).*session_locks|time_to_live|max_capacity' -C 5 echo "=== Locate chat_locks (guideline expectation) ===" rg -n --type rust '\bchat_locks\b' -C 3 echo "=== Locate resolve_encryption_jid usage + lock scope ===" rg -n --type rust 'resolve_encryption_jid|to_protocol_address\(\)|session_locks\.get_with' -C 3Also applies to: 299-307
src/message.rs (1)
64-177: Fix misleading “Cached … mapping” logs (they currently log success even on persist failure) + consider redacting PII in warn logs.
Right now both branches emitdebug!("Cached … mapping")even ifadd_lid_pn_mapping(...).awaitreturnsErr(same issue as prior feedback). Also,warn!logs include raw phone/LID identifiers (PII).- if let Err(err) = self + match self .add_lid_pn_mapping( @@ - .await - { - warn!( - "Failed to persist LID-to-PN mapping {} -> {}: {err}", - sender.user, alt_jid.user - ); - } - debug!( - "Cached LID-to-PN mapping: {} -> {}", - sender.user, alt_jid.user - ); + .await + { + Ok(()) => debug!("Cached LID-to-PN mapping: {} -> {}", sender.user, alt_jid.user), + Err(err) => warn!("Failed to persist LID-to-PN mapping {} -> {}: {err}", sender.user, alt_jid.user), + }(Apply the same pattern to the PN→LID block.)
For the PII angle, consider masking (e.g., last 4) or logging a stable hash inwarn!paths.
🧹 Nitpick comments (4)
storages/sqlite-storage/src/sqlite_store.rs (2)
89-93: Comment is misleading: increasing pool size increases memory usage.The comment claims "reduce memory and lock contention," but raising
max_sizefrom 4 to 64 increases memory footprint (64 connections vs. 4). The intent appears to be increasing concurrency, not reducing memory.- .max_size(pool_size) // Limit concurrent connections to reduce memory and lock contention + .max_size(pool_size) // Increased to support higher concurrency
794-859: Approve retry logic; consider minor efficiency improvement.The exponential backoff pattern correctly handles SQLite contention. The semaphore-per-attempt approach is sound.
Minor efficiency:
address_ownedandkey_vecare cloned on every retry iteration (lines 807-808). Since they're already owned outside the loop, you could move the.clone()calls outside and reuse them:+ let address_owned = address.to_string(); + let key_vec = key.to_vec(); + for attempt in 0..=MAX_RETRIES { // ... - let address_clone = address_owned.clone(); - let key_clone = key_vec.clone(); + let address_clone = address_owned.clone(); // Still needed per spawn_blocking + let key_clone = key_vec.clone();Actually, on second look, this is already optimal since spawn_blocking moves ownership. The current pattern is fine.
src/message.rs (2)
367-380: Session lock is a good direction, but watch unbounded growth + ensure this complementsClient::chat_locks(per repo guidance).
self.session_locks.get_with(signal_addr_str, …)will grow without eviction as new senders/devices appear. Also, repo guidance forsrc/message.rsis to serialize per-chat operations viaClient::chat_locks; this adds per-session serialization, but shouldn’t replace per-chat ordering guarantees if callers rely on them.Consider an eviction strategy (TTL/LRU) for
session_lockskeys, and double-check chat-level serialization where needed. Based on learnings, useClient::chat_locksfor per-chat serialization.
2551-3125: New cache population tests: watch flakiness from spawned background tasks.
These tests callhandle_encrypted_message, which spawns tasks (delivery receipts, retry receipts, PDO requests). That can lead to nondeterministic logs/timing in CI.If you see flakes, consider injecting a “test mode” that disables side-effect spawns, or await/join spawned tasks via a task tracker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/message.rs(10 hunks)src/prekeys.rs(4 hunks)src/send.rs(8 hunks)src/socket/error.rs(2 hunks)src/socket/noise_socket.rs(6 hunks)storages/sqlite-storage/src/sqlite_store.rs(24 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/send.rssrc/socket/error.rssrc/prekeys.rssrc/socket/noise_socket.rssrc/message.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/send.rssrc/message.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
src/send.rssrc/socket/error.rssrc/prekeys.rssrc/socket/noise_socket.rssrc/message.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/send.rssrc/socket/noise_socket.rssrc/message.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to {src,wacore}/**/*.rs : Use anyhow::Error for functions with multiple failure modes
Applied to files:
src/send.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to {src,wacore}/**/*.rs : Use thiserror for custom error types (e.g., SocketError)
Applied to files:
src/socket/error.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
storages/sqlite-storage/src/sqlite_store.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
storages/sqlite-storage/src/sqlite_store.rs
🧬 Code graph analysis (3)
src/send.rs (3)
src/client.rs (1)
new(171-270)wacore/src/send.rs (2)
new(850-856)prepare_peer_stanza(463-502)src/store/persistence_manager.rs (1)
new(20-62)
src/prekeys.rs (1)
src/store/persistence_manager.rs (1)
backend(120-122)
src/socket/noise_socket.rs (2)
src/socket/error.rs (2)
transport(69-80)channel_closed(91-98)wacore/src/handshake/utils.rs (1)
generate_iv(37-41)
🔇 Additional comments (11)
src/prekeys.rs (1)
98-105: LGTM: Good pattern for reducing lock contention.Cloning the backend Arc and dropping the guard early is a sound approach for the potentially long-running pre-key operations that follow. This allows concurrent access to the device store while maintaining safe access to the backend.
storages/sqlite-storage/src/sqlite_store.rs (4)
932-948: LGTM! Clear debug logging with helpful context.The logging and comments effectively explain the normal concurrent behavior during session creation, which helps prevent false alarms.
1067-1112: LGTM! Batch query correctly chunked to avoid SQLite parameter limits.The 900-item chunks safely stay under SQLite's bind parameter limit (typically 999), and returning a
HashSetenables efficient lookup.
41-41: LGTM!edge_routing_infoproperly propagated through device persistence.The
Option<Vec<u8>>field is consistently saved, loaded, and defaulted across all device data paths.Also applies to: 231-231, 274-274, 294-294, 430-430, 495-495
2186-2213: LGTM! Trait implementation correctly delegates to device-scoped methods.The single-device mode (device_id = 1) delegation is consistent with other trait implementations in this file.
src/socket/error.rs (1)
91-98: Good: dedicatedchannel_closedconstructor preserves buffers and gives a clear discriminant.
Having a first-class kind for “sender task died / channel closed” should make higher-level retries/telemetry cleaner than overloadingTransport.src/send.rs (2)
11-20: LGTM: switching send path to&wa::Messagereduces Arc churn and aligns with wacore APIs.
Just ensure no spawned tasks capturemessageby reference beyond thesend_messageawait boundary.Also applies to: 22-35
83-86: Theadd_recent_messageimplementation correctly handles borrowed message references.With
message: &wa::Message, the method serializes immediately to ownedVec<u8>viamsg.encode_to_vec()at line 654, then stores this in the moka cache at line 655. No dangling references. The moka cache itself handles thread-safety for concurrent access, so no additional per-chat locks are needed for this caching operation.src/socket/noise_socket.rs (1)
38-90: Nice improvement: single sender task guarantees frame order without a mutex.
Sequential job processing +spawn_blockingfor large encryptions matches the Tokio I/O guidance well.Also applies to: 91-175
src/message.rs (2)
251-273: Replace substring matching for group/broadcast detection with explicit server equality checks.The
is_group_senderheuristic usesserver.contains(".us")which is problematic. This matches any server ending in ".us" (including "c.us" for regular users), not just group servers ("g.us") and broadcast servers. If user JIDs can have a server value of "c.us", session decryption will be incorrectly skipped.Replace with explicit equality checks:
server == "g.us" || server == "broadcast"or, if available, use semantic methods likesender_encryption_jid.is_group()orsender_encryption_jid.is_broadcast().
381-429: The code is correct for rand 0.9 withTryRngCore. The.unwrap_err()call is a valid method from theTryRngCoretrait (properly imported at the top of the file), which is the idiomatic way to useOsRngin rand 0.9. The pattern&mut rng.unwrap_err()and&mut rand::rngs::OsRng.unwrap_err()are both valid and compile correctly. No changes needed.Likely an incorrect or invalid review comment.
… update LID mapping logging
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/message.rs (1)
93-110: Debug logs incorrectly report success even when persistence fails.The debug log at lines 106-109 executes unconditionally after the
if let Errblock, so it will log "Cached LID-to-PN mapping" even ifadd_lid_pn_mappingreturned an error. The same issue occurs at lines 120-136.Apply this diff to gate the debug log on success:
- if let Err(err) = self + match self .add_lid_pn_mapping( &sender.user, &alt_jid.user, crate::lid_pn_cache::LearningSource::PeerLidMessage, ) .await { - warn!( - "Failed to persist LID-to-PN mapping {} -> {}: {err}", - sender.user, alt_jid.user - ); + Ok(()) => debug!( + "Cached LID-to-PN mapping: {} -> {}", + sender.user, alt_jid.user + ), + Err(err) => warn!( + "Failed to persist LID-to-PN mapping {} -> {}: {err}", + sender.user, alt_jid.user + ), } - debug!( - "Cached LID-to-PN mapping: {} -> {}", - sender.user, alt_jid.user - );Apply the same pattern to lines 120-136.
🧹 Nitpick comments (3)
src/socket/noise_socket.rs (1)
38-62: Constructor correctly spawns the sender task.The initialization properly uses
Arcfor shared ownership and spawns the sender task with appropriate Tokio primitives. No error handling issues.Optional: Consider documenting the rationale for the channel buffer size of 32 (line 45):
// Create channel for send jobs. Buffer size of 32 allows multiple -// callers to enqueue work without blocking on channel capacity. +// callers to enqueue work without blocking on channel capacity. +// 32 is chosen to balance memory usage with throughput for typical workloads. let (send_job_tx, send_job_rx) = mpsc::channel::<SendJob>(32);src/handlers/message.rs (1)
79-84: Remove redundant.clone()call.
client_for_workeris already anArc<Client>clone, so calling.clone()again on line 82 is redundant (though harmless).Apply this diff to simplify:
tokio::spawn(async move { while let Some(msg_node) = rx.recv().await { - client_for_worker - .clone() - .handle_encrypted_message(msg_node) + client_for_worker.handle_encrypted_message(msg_node) .await; }src/message.rs (1)
251-254: Fragile string matching for server type detection.The check
sender_encryption_jid.server.contains(".us")could theoretically match unintended server names. Consider using exact server constant comparisons (e.g.,wacore_binary::jid::GROUP_SERVER,wacore_binary::jid::BROADCAST_SERVER) instead of substring matching for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/handlers/message.rs(3 hunks)src/message.rs(10 hunks)src/socket/noise_socket.rs(6 hunks)src/usync.rs(2 hunks)storages/sqlite-storage/src/sqlite_store.rs(24 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/usync.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: All I/O in the main crate must use Tokio; be mindful of race conditions
Wrap blocking I/O (e.g., ureq) and heavy CPU-bound tasks (e.g., media encryption) in tokio::task::spawn_blocking
Avoid .unwrap() and .expect() outside of tests and truly unrecoverable paths
Files:
src/handlers/message.rssrc/socket/noise_socket.rssrc/message.rs
{src,wacore}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,wacore}/**/*.rs: Use thiserror for custom error types (e.g., SocketError)
Use anyhow::Error for functions with multiple failure modes
Files:
src/handlers/message.rssrc/socket/noise_socket.rssrc/message.rs
src/{client,send,message}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Use Client::chat_locks to serialize per-chat operations
Files:
src/message.rs
🧠 Learnings (3)
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to src/{client,send,message}.rs : Use Client::chat_locks to serialize per-chat operations
Applied to files:
src/handlers/message.rssrc/message.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: The whatsapp-rust (main) crate integrates wacore with Tokio for async and Diesel for SQLite persistence
Applied to files:
storages/sqlite-storage/src/sqlite_store.rs
📚 Learning: 2025-09-20T02:07:36.999Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-20T02:07:36.999Z
Learning: Applies to wacore/**/*.rs : wacore must remain no_std-compatible and have no dependencies on Tokio or specific databases
Applied to files:
storages/sqlite-storage/src/sqlite_store.rs
🧬 Code graph analysis (3)
src/handlers/message.rs (2)
src/handlers/traits.rs (1)
handle(26-26)src/client.rs (1)
new(171-270)
src/socket/noise_socket.rs (3)
src/socket/error.rs (2)
transport(69-80)channel_closed(91-98)wacore/src/handshake/utils.rs (1)
generate_iv(37-41)src/transport.rs (2)
send(22-24)disconnect(26-26)
src/message.rs (3)
wacore/binary/src/attrs.rs (2)
jid(75-78)jid(201-204)wacore/binary/src/jid.rs (9)
user(219-219)user(290-292)user(347-349)from(213-215)from(557-559)from(563-565)server(220-220)server(293-295)server(350-352)src/lid_pn_cache.rs (1)
get_current_lid(156-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (27)
storages/sqlite-storage/src/sqlite_store.rs (12)
8-8: LGTM: Imports and type changes support new functionality.The new imports (
warn,Arc) and theedge_routing_infofield addition toDeviceRoware appropriate for the retry logic and edge routing feature additions.Also applies to: 10-10, 15-15, 17-17, 41-42
89-93: Pool and semaphore sizing aligned at 64.The pool and semaphore are now consistently set to 64, which addresses previous review feedback. However, 64 concurrent SQLite connections is quite aggressive and may stress the database under certain workloads.
Consider monitoring for "database is locked" errors or high memory usage under peak load.
Also applies to: 122-122
65-65: LGTM: PRAGMA tuning and foreign key enforcement.
- Doubling
busy_timeoutto 30 seconds provides more tolerance for write contention.- Enabling
foreign_keysis essential for referential integrity, especially with the newlid_pn_mappingtable.Also applies to: 77-80
231-231: LGTM: Consistent edge_routing_info propagation.The
edge_routing_infofield is consistently propagated through all device save/load paths with appropriate handling of theOption<Vec<u8>>type.Also applies to: 274-274, 294-294, 330-330, 365-365, 385-385, 430-430, 495-495, 553-553, 724-724, 779-779
794-859: LGTM: Retry logic with exponential backoff for identity writes.The retry logic appropriately handles SQLite write contention with exponential backoff (10→20→40→80→160ms) and warning logs for debugging.
963-1035: LGTM: Retry logic with exponential backoff for session writes.Consistent with the identity write retry pattern, with appropriate debug logging for successful session persistence.
863-880: LGTM: Refactored read/delete operations with consistent semaphore usage.The read operations now use the
with_semaphorehelper for consistent concurrency control. The extensive debug logging inget_session_for_device(lines 932-948) helpfully explains that missing sessions during concurrent message processing is normal behavior.Also applies to: 889-909, 916-954, 1039-1057
1067-1112: LGTM: Batch session checking with proper chunking.The batch API correctly chunks queries at 900 addresses to stay well below SQLite's bind parameter limit (~999), addressing previous review feedback.
1995-2033: LGTM: LID-PN mapping lookups handle both directions.The lookup methods properly query by LID or phone number, with the phone lookup correctly ordering by
updated_at DESCto retrieve the most recent mapping (line 2057), and the comment at line 2046 accurately describes this behavior.Also applies to: 2035-2075
2077-2122: LGTM: LID-PN mapping upsert correctly preserves created_at.The upsert logic properly:
- Sets
created_atonly on initial insert (line 2103)- Updates only
phone_number,learning_source, andupdated_aton conflict (lines 2110-2113)This addresses previous review feedback about not overwriting
created_aton updates.
2124-2161: LGTM: LID-PN mapping retrieval and deletion.The
get_allanddeleteoperations follow the established pattern for device-scoped queries.Also applies to: 2163-2183
2185-2212: LGTM: LidPnMappingStore trait delegates to device_id=1.The trait implementation follows the established pattern of delegating to device-specific methods with
device_id=1for single-device mode, consistent with other trait implementations in this file.src/socket/noise_socket.rs (7)
1-23: LGTM: Clean async primitives and type definitions.The imports and type definitions properly set up the task-based send pipeline.
SendJobencapsulates the work unit with buffers and a response channel, whileSendResultclearly defines the success/error return signature.
25-36: LGTM: Well-structured ownership model.The refactored struct properly delegates write operations to a dedicated sender task via an MPSC channel, avoiding lock contention while maintaining sequential ordering guarantees.
64-89: LGTM: Sequential processing ensures ordering.The sender task correctly owns
write_counterand processes jobs one at a time, guaranteeing FIFO ordering and monotonic counter increments without synchronization overhead. The graceful shutdown on channel closure is proper.
91-174: LGTM: Correct encryption and framing logic with proper async handling.The function correctly:
- Owns and increments the write counter before encryption
- Uses in-place encryption for small messages to avoid allocations
- Offloads large-message encryption to
spawn_blocking(complies with coding guidelines)- Preserves buffer ownership through all error paths for caller reuse
- Uses async transport correctly
Line 145's
unwrap_or_elsefallback is defensive—try_unwrapshould always succeed since the spawned blocking task has completed—but the defensive clone is harmless.
176-203: LGTM: Buffer recovery now correctly implemented.Lines 186-193 properly recover the buffers from the
SendErrorwhen the channel send fails, addressing the previous review concern. The caller can now reuse their allocated capacity even on channel-closed errors.Lines 198-200: Returning empty
Vecs when the oneshot receive fails is unavoidable—the sender task holds the buffers and has either panicked or been aborted (e.g., duringDrop). This is an acceptable edge case for abnormal shutdown scenarios.
214-221: LGTM: Proper cleanup via task abort.Aborting the sender task on drop prevents resource leaks if the transport hangs. In-flight
encrypt_and_sendcalls will fail gracefully with empty buffers (acceptable for abnormal shutdown).
278-354: LGTM: Test now correctly verifies FIFO ordering.The test addresses the previous review concern by:
- Embedding a unique index in each message's plaintext (line 337)
- Using a
RecordingTransportthat decrypts and extracts the index from each sent frame (lines 304-310)- Asserting the recorded send order matches the expected sequence
[0, 1, 2, ..., 9](lines 350-353)This validates that the sender task maintains FIFO ordering across concurrent
encrypt_and_sendcalls.src/handlers/message.rs (2)
48-62: Well-designed concurrency control.The enqueue lock pattern correctly ensures message ordering even when the queue is created concurrently. The lock is held for the entire enqueue operation (including queue creation), preventing race conditions.
94-97: Message loss on enqueue failure is logged but not recovered.If
tx.send(node).awaitfails (e.g., channel closed), the message is logged but lost. This is acceptable if the channel should never close during normal operation, but ensure upstream error handling or monitoring can detect this condition.src/message.rs (6)
367-379: Well-designed per-sender session locking.Using the full Signal protocol address string as the lock key correctly prevents race conditions when multiple messages from the same sender are processed concurrently, and aligns with
SignalProtocolStoreAdapter's internal locking.
472-486: Correctly avoids holding lock across async I/O.The backend handle is properly extracted while holding the lock (lines 472-478), then the lock is dropped before the async
delete_identitycall (line 486). This avoids the lock contention issue flagged in the previous review.
938-950: Correct bidirectional sender_alt extraction.The logic properly handles both LID senders (looking for
sender_pn) and PN senders (looking forsender_lid), enabling bidirectional session resolution regardless of which address format is used.
1668-1725: Comprehensive test coverage for LID protocol address format.This test validates that the protocol address generation matches WhatsApp Web's format, which is critical for session compatibility. The test cases cover various edge cases including multiple dots and missing device numbers.
2554-2794: Excellent test coverage for LID-PN cache behavior.These test cases thoroughly validate the cache population logic for various scenarios:
- DM messages with
sender_lidattribute- Messages without
sender_lid(negative case)- LID senders with
participant_pnin groups- Repeated messages (idempotency)
The documentation clearly explains the purpose and expected behavior of each test.
2813-3128: Thorough validation of session resolution logic.These tests cover all three session resolution paths comprehensively:
- Using LID from
sender_lidattribute (lines 2813-2926)- Using cached LID mapping when attribute absent (lines 2928-3035)
- Falling back to PN when no mapping exists (lines 3037-3128)
The tests validate the critical fix for MAC verification failures by ensuring the Rust client matches WhatsApp Web's session lookup behavior.
| .get_with_by_ref(&chat_id, async { | ||
| // Create a channel with backpressure | ||
| // Increased capacity to handle high message rates without blocking | ||
| let (tx, mut rx) = mpsc::channel::<Arc<Node>>(10000); |
There was a problem hiding this comment.
Consider potential memory pressure from large channel capacity.
The capacity of 10,000 messages per chat, combined with up to 10,000 cached chats, could result in significant memory usage (potentially millions of messages in memory) under high load scenarios. While the 5-minute TTL helps expire idle queues and each message is just an Arc<Node>, consider whether this capacity is necessary or if a lower value (e.g., 1000) with appropriate backpressure handling would be safer.
Carefully has some breaking changes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.