refactor: split message/client/send monoliths into per-theme modules - #732
Conversation
Pure code movement with no logic changes. Decomposes the three largest source files (src/message.rs 12467 LOC, src/client.rs 7117 LOC, wacore/src/send.rs 5322 LOC) into module directories grouped by theme, following the existing src/client/ submodule pattern. The only non-move edits are private to pub(crate) visibility bumps for items now referenced across modules or from tests, plus re-export lines. No public API changes.
src/message.rs (167 LOC root) keeps the helper structs, free fns and the RetryReason re-export; the impl Client decrypt/receive pipeline moves into message/{dispatch,msg_secret,retry,receive,special}.rs and the tests into message/tests.rs.
src/client.rs (810 LOC root) keeps the Client struct, public types (NodeFilter, ClientError, MemoryDiagnostics) and the ack free fns; the impl Client methods move into client/{lifecycle,node_io,app_state,iq_ops,adapters,messaging,accessors}.rs and the tests into client/tests.rs.
wacore/src/send.rs (84 LOC root) keeps mod stanza, StanzaType and the re-exports that preserve the wacore::send::* paths; the free fns move into send/{classify,encrypt,dm,peer,group,status}.rs and the tests into send/tests.rs.
Verified with cargo fmt --all, cargo clippy --all-targets -- -D warnings (clean) and cargo test --workspace --exclude e2e-tests (3975 passed, 0 failed). Test counts preserved: 147 message, 73 client, 89 send.
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughImplements a complete client runtime: lifecycle and reconnect logic, transport read loop and stanza routing, a two-pass incoming-message decryption pipeline with retry/secret handling, outgoing stanza builders and per-device encryption, app-state sync, accessors/adapters/IQ ops, and a broad test suite. ChangesComplete Client Messaging Pipeline
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Note: review the transport/read-loop and decrypt retry ordering carefully — these areas must be correct for reliability. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Benchmark Results67 unchanged benchmark(s)
|
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 50s —— View job Claude PR Review — pure code-movement refactor
VerdictLGTM. This is exactly what the description says: a no-logic, pure-relocation refactor. All 5 CI runs (Rust CI, WASM Build, Benchmark, Integration Benchmark, E2E Tests) are green, and benchmark deltas are at noise level (largest is +0.5% on What I verified
Observations / non-blocking notes
Bottom lineClean low-risk move. No logic changes detected, re-exports are correct, CI is green, benchmarks are flat. Ship it. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/app_state.rs`:
- Around line 480-493: Extract the duplicated download logic into a small helper
so both places reuse the same implementation: add a helper like
get_pre_downloaded_blob(pre_downloaded: &PreDownloadedMap, ext:
&wa::ExternalBlobReference) -> anyhow::Result<Vec<u8>> (or a factory fn
make_download_closure(pre_downloaded: &PreDownloadedMap) -> impl
Fn(&wa::ExternalBlobReference) -> anyhow::Result<Vec<u8>>), then replace the
inline download closure here and the similar closure in
sync_collections_batched_inner with calls to that helper; ensure it checks
ext.direct_path, looks up pre_downloaded by path, returns the bytes clone on hit
or an anyhow error with the same messages ("external blob not pre-downloaded:
{}" and "external blob has no directPath") so behavior remains identical.
- Around line 622-632: The IQ being sent from this patch uses
crate::request::InfoQuery without a timeout, so add a 30-second timeout to the
InfoQuery before calling self.send_iq(iq). Specifically, set the
InfoQuery.timeout field to Some(Duration::from_secs(30)) (importing
std::time::Duration if needed) in the block that builds the iq (the InfoQuery
instance used with self.send_iq), mirroring the existing 30s timeout pattern
used elsewhere.
- Around line 411-419: The IQ being constructed in app_state.rs (variable iq of
type crate::request::InfoQuery) currently sets timeout: None which can hang the
sync; update the iq timeout field to a sensible timeout (e.g.
Some(Duration::from_secs(30))) consistent with the batched sync path so the
InfoQuery has a finite wait; ensure you import or reference std::time::Duration
if needed and apply the change where iq is created so timeout is not None.
In `@src/client/messaging.rs`:
- Around line 174-178: The warning logs the client's UUID (self.unique_id)
instead of the failed receipt's ID; locate the block around the
self.send_node(node).await call in messaging.rs and change the log to include
the receipt's identifier (e.g., receipt_id or receipt.unique_id — whichever
variable holds the receipt ID in this scope) in place of self.unique_id so the
message reads the failed receipt ID, keeping receipt_type and the error e as
before.
In `@src/client/node_io.rs`:
- Around line 396-414: The deferred-ACK branch currently bypasses outbound_flush
by using runtime.spawn directly, allowing disconnect() to close the transport
while the ACK is in flight; update maybe_deferred_ack so the ACK task is
submitted/tracked through the same outbound_flush mechanism used for other
outbound work (instead of calling runtime.spawn directly). Concretely, replace
the runtime.spawn(Box::pin(...)) usage in maybe_deferred_ack with the codepath
that enqueues or registers the async closure/future with outbound_flush (so
disconnect() will wait for it), preserving the existing
send_ack_for(node.get()).await and the transport-unavailable check/logging.
Ensure you reference maybe_deferred_ack, send_ack_for, outbound_flush and
disconnect when making the change.
In `@src/message/receive.rs`:
- Around line 841-847: The code assigns SessionBatchOutcome::undecryptable from
the return of handle_decrypt_failure, which overwrites previous true values for
earlier payloads; change these direct assignments (e.g., outcome.undecryptable =
self.handle_decrypt_failure(...).await) to accumulate using |= so undecryptable
remains true if any payload set it. Update every retry/decrypt-failure site that
currently assigns into outcome.undecryptable (including the occurrences around
the handle_decrypt_failure calls shown and the other similar blocks noted) to
use outcome.undecryptable |= self.handle_decrypt_failure(...).await; this
mirrors the parse-error branches and prevents emitting duplicate
process_classified_message events.
- Around line 1519-1526: The code currently calls
self.persistence_manager.get_device_arc() and reads guard.pn/guard.lid directly;
replace that with a call to
self.persistence_manager.get_device_snapshot().await, then extract snapshot.pn
and snapshot.lid (use the same default_jid fallback for pn and pass
lid.as_ref()) and feed those into wacore::messages::parse_message_info(...) so
parsing uses the repository snapshot instead of the internal Device Arc; update
references from get_device_arc, guard, own_pn and own_lid to get_device_snapshot
and snapshot.pn/snapshot.lid.
In `@src/message/special.rs`:
- Around line 24-41: The code currently bails silently when
plaintext_node.content_bytes() returns None; add a log entry to surface that
case: locate the block around plaintext_node.content_bytes(), and when it yields
None emit a debug or warn log (including info.id and info.source.chat)
explaining that the plaintext node existed but had no content bytes so decoding
was skipped; leave the existing wa::Message::decode and
self.dispatch_parsed_message(...) behavior unchanged for the Some(bytes) path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fcbabcef-edf2-4878-90f5-b6f329b79d51
📒 Files selected for processing (24)
src/client.rssrc/client/accessors.rssrc/client/adapters.rssrc/client/app_state.rssrc/client/iq_ops.rssrc/client/lifecycle.rssrc/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/message.rssrc/message/dispatch.rssrc/message/msg_secret.rssrc/message/receive.rssrc/message/retry.rssrc/message/special.rssrc/message/tests.rswacore/src/send.rswacore/src/send/classify.rswacore/src/send/dm.rswacore/src/send/encrypt.rswacore/src/send/group.rswacore/src/send/peer.rswacore/src/send/status.rswacore/src/send/tests.rs
| let iq = crate::request::InfoQuery { | ||
| namespace: "w:sync:app:state", | ||
| query_type: crate::request::InfoQueryType::Set, | ||
| to: server_jid().clone(), | ||
| target: None, | ||
| id: None, | ||
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | ||
| timeout: None, | ||
| }; |
There was a problem hiding this comment.
Missing timeout on IQ request creates hang risk.
Look, we've got timeout: Some(Duration::from_secs(30)) in the batched sync path (line 205), but here we're sending IQ requests with timeout: None. If the server decides to ghost us, this entire sync operation just... hangs. Forever. That's not how we build resilient systems at scale.
The pagination cap at line 384 protects against infinite loops from has_more_patches, but it won't save us from a single stuck request blocking the entire sync pipeline.
🔧 Proposed fix: Add consistent timeout
let iq = crate::request::InfoQuery {
namespace: "w:sync:app:state",
query_type: crate::request::InfoQueryType::Set,
to: server_jid().clone(),
target: None,
id: None,
content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])),
- timeout: None,
+ timeout: Some(Duration::from_secs(30)),
};📝 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.
| let iq = crate::request::InfoQuery { | |
| namespace: "w:sync:app:state", | |
| query_type: crate::request::InfoQueryType::Set, | |
| to: server_jid().clone(), | |
| target: None, | |
| id: None, | |
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | |
| timeout: None, | |
| }; | |
| let iq = crate::request::InfoQuery { | |
| namespace: "w:sync:app:state", | |
| query_type: crate::request::InfoQueryType::Set, | |
| to: server_jid().clone(), | |
| target: None, | |
| id: None, | |
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | |
| timeout: Some(Duration::from_secs(30)), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/app_state.rs` around lines 411 - 419, The IQ being constructed in
app_state.rs (variable iq of type crate::request::InfoQuery) currently sets
timeout: None which can hang the sync; update the iq timeout field to a sensible
timeout (e.g. Some(Duration::from_secs(30))) consistent with the batched sync
path so the InfoQuery has a finite wait; ensure you import or reference
std::time::Duration if needed and apply the change where iq is created so
timeout is not None.
| let download = |ext: &wa::ExternalBlobReference| -> anyhow::Result<Vec<u8>> { | ||
| if let Some(path) = &ext.direct_path { | ||
| if let Some(bytes) = pre_downloaded.get(path) { | ||
| Ok(bytes.clone()) | ||
| } else { | ||
| Err(anyhow::anyhow!( | ||
| "external blob not pre-downloaded: {}", | ||
| path | ||
| )) | ||
| } | ||
| } else { | ||
| Err(anyhow::anyhow!("external blob has no directPath")) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting duplicate download closure logic.
This closure is a near-exact copy of lines 259-272 in sync_collections_batched_inner. When we copy-paste code, we're basically setting ourselves up for a future where one gets fixed and the other doesn't.
A small helper function that takes the pre_downloaded map and returns the closure (or directly performs the lookup) would keep things DRY.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/app_state.rs` around lines 480 - 493, Extract the duplicated
download logic into a small helper so both places reuse the same implementation:
add a helper like get_pre_downloaded_blob(pre_downloaded: &PreDownloadedMap,
ext: &wa::ExternalBlobReference) -> anyhow::Result<Vec<u8>> (or a factory fn
make_download_closure(pre_downloaded: &PreDownloadedMap) -> impl
Fn(&wa::ExternalBlobReference) -> anyhow::Result<Vec<u8>>), then replace the
inline download closure here and the similar closure in
sync_collections_batched_inner with calls to that helper; ensure it checks
ext.direct_path, looks up pre_downloaded by path, returns the bytes clone on hit
or an anyhow error with the same messages ("external blob not pre-downloaded:
{}" and "external blob has no directPath") so behavior remains identical.
| let iq = crate::request::InfoQuery { | ||
| namespace: "w:sync:app:state", | ||
| query_type: crate::request::InfoQueryType::Set, | ||
| to: server_jid().clone(), | ||
| target: None, | ||
| id: None, | ||
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | ||
| timeout: None, | ||
| }; | ||
|
|
||
| self.send_iq(iq).await?; |
There was a problem hiding this comment.
Same missing timeout issue on patch send IQ.
Same story here - we're sending an IQ request with no timeout. If the server doesn't respond after we push our patch, we're stuck. Consistency matters. Let's use the same 30-second timeout pattern.
🔧 Proposed fix
let iq = crate::request::InfoQuery {
namespace: "w:sync:app:state",
query_type: crate::request::InfoQueryType::Set,
to: server_jid().clone(),
target: None,
id: None,
content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])),
- timeout: None,
+ timeout: Some(Duration::from_secs(30)),
};📝 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.
| let iq = crate::request::InfoQuery { | |
| namespace: "w:sync:app:state", | |
| query_type: crate::request::InfoQueryType::Set, | |
| to: server_jid().clone(), | |
| target: None, | |
| id: None, | |
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | |
| timeout: None, | |
| }; | |
| self.send_iq(iq).await?; | |
| let iq = crate::request::InfoQuery { | |
| namespace: "w:sync:app:state", | |
| query_type: crate::request::InfoQueryType::Set, | |
| to: server_jid().clone(), | |
| target: None, | |
| id: None, | |
| content: Some(wacore_binary::NodeContent::Nodes(vec![sync_node])), | |
| timeout: Some(Duration::from_secs(30)), | |
| }; | |
| self.send_iq(iq).await?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/app_state.rs` around lines 622 - 632, The IQ being sent from this
patch uses crate::request::InfoQuery without a timeout, so add a 30-second
timeout to the InfoQuery before calling self.send_iq(iq). Specifically, set the
InfoQuery.timeout field to Some(Duration::from_secs(30)) (importing
std::time::Duration if needed) in the block that builds the iq (the InfoQuery
instance used with self.send_iq), mirroring the existing 30s timeout pattern
used elsewhere.
| async fn maybe_deferred_ack(self: &Arc<Self>, node: Arc<wacore_binary::OwnedNodeRef>) { | ||
| if self.synchronous_ack { | ||
| if let Err(e) = self.send_ack_for(node.get()).await | ||
| && !e.is_transport_unavailable() | ||
| { | ||
| warn!("Failed to send ack: {e:?}"); | ||
| } | ||
| } else { | ||
| let this = self.clone(); | ||
| self.runtime | ||
| .spawn(Box::pin(async move { | ||
| if let Err(e) = this.send_ack_for(node.get()).await | ||
| && !e.is_transport_unavailable() | ||
| { | ||
| warn!("Failed to send ack: {e:?}"); | ||
| } | ||
| })) | ||
| .detach(); | ||
| } |
There was a problem hiding this comment.
Track deferred ACK sends through outbound_flush.
The async branch goes straight through runtime.spawn(). disconnect() only waits on outbound_flush, so cleanup can close the transport while this ACK is still in flight. That drops the ACK under disconnect pressure and reopens the replay / <stream:error><ack/> path.
Suggested fix
} else {
let this = self.clone();
- self.runtime
- .spawn(Box::pin(async move {
- if let Err(e) = this.send_ack_for(node.get()).await
- && !e.is_transport_unavailable()
- {
- warn!("Failed to send ack: {e:?}");
- }
- }))
- .detach();
+ self.outbound_flush.spawn(&*self.runtime, async move {
+ if let Err(e) = this.send_ack_for(node.get()).await
+ && !e.is_transport_unavailable()
+ {
+ warn!("Failed to send ack: {e:?}");
+ }
+ });
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/node_io.rs` around lines 396 - 414, The deferred-ACK branch
currently bypasses outbound_flush by using runtime.spawn directly, allowing
disconnect() to close the transport while the ACK is in flight; update
maybe_deferred_ack so the ACK task is submitted/tracked through the same
outbound_flush mechanism used for other outbound work (instead of calling
runtime.spawn directly). Concretely, replace the runtime.spawn(Box::pin(...))
usage in maybe_deferred_ack with the codepath that enqueues or registers the
async closure/future with outbound_flush (so disconnect() will wait for it),
preserving the existing send_ack_for(node.get()).await and the
transport-unavailable check/logging. Ensure you reference maybe_deferred_ack,
send_ack_for, outbound_flush and disconnect when making the change.
| let (own_pn, own_lid) = { | ||
| let arc = self.persistence_manager.get_device_arc().await; | ||
| let guard = arc.read().await; | ||
| (guard.pn.clone(), guard.lid.clone()) | ||
| }; | ||
| let default_jid = Jid::default(); | ||
| let own_jid = own_pn.as_ref().unwrap_or(&default_jid); | ||
| wacore::messages::parse_message_info(node, own_jid, own_lid.as_ref()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the device snapshot here.
This read path reaches into get_device_arc() just to pull pn and lid. That bypasses the repository contract and couples message parsing to Device internals for no gain. Read a snapshot instead.
As per coding guidelines, use get_device_snapshot() for Device reads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/message/receive.rs` around lines 1519 - 1526, The code currently calls
self.persistence_manager.get_device_arc() and reads guard.pn/guard.lid directly;
replace that with a call to
self.persistence_manager.get_device_snapshot().await, then extract snapshot.pn
and snapshot.lid (use the same default_jid fallback for pn and pass
lid.as_ref()) and feed those into wacore::messages::parse_message_info(...) so
parsing uses the repository snapshot instead of the internal Device Arc; update
references from get_device_arc, guard, own_pn and own_lid to get_device_snapshot
and snapshot.pn/snapshot.lid.
Source: Coding guidelines
…s, uniformity, test)
Follow-up to the monolith split addressing the Codex review. None of these change the runtime path: they fix logging, normalize an accumulator, and add batch coverage.
messaging.rs: send_protocol_receipt logged self.unique_id (the client UUID) under a "message ID {}" label; log the receipt id instead. The id is read by borrow (.attr("id", id.as_str())) so it stays available for the error log without an extra clone.
message/receive.rs: normalize the 5 remaining `outcome.undecryptable = handle_decrypt_failure(...)` sites to `|=`, matching the other 10 accumulator sites. handle_decrypt_failure always returns true today, so this is inert; the `|=` keeps every undecryptable accumulation site uniform and avoids a clobber footgun if that return ever becomes dedup-aware.
message/special.rs: log at debug when a newsletter <plaintext> node has no content bytes (was silently skipped).
tests: add a batch-level invariant test where two undecryptable payloads sharing one (chat,id) accumulate `undecryptable` and dispatch exactly one UndecryptableMessage (single-flight dedup through process_session_enc_batch).
Review items intentionally skipped: the two IQ "missing timeout" nits (send_iq applies a 75s default when timeout is None, so no hang) and switching parse_message_info to get_device_snapshot (the snapshot clones the whole Device, while the per-message hot path intentionally clones only pn/lid).
Resolves the stale PR by merging current main and finishing the protobuf codegen migration from prost to buffa. Tracks the tagged upstream release anthropics/buffa v0.8.0 (the jlucaso1 fork is dropped now that oneof_attribute is upstream). The .proto stays in the upstream/whatspec camelCase form; build.rs snake_cases the committed descriptor for the Rust API via buffa-descriptor, so consumers never need protoc. Zero prost: the sqlite-storage on-disk blobs and the voip/mlow runtime constant tables now generate buffa types from their own .proto + checked-in descriptor (wire-compatible by field number, no protoc for consumers); the manual DecryptionErrorMessageProto codec moves to the buffa Message trait; and prost/prost-build/prost-types are removed from the workspace. main's #732 monolith splits (wacore send; src client/message/send) are adopted over the branch's pre-split monoliths, and the whole src/ crate is migrated to the buffa API: MessageField presence/accessors, decode_from_slice, typed enum fields, ADV* type casing, and snake_case oneof fields.
This is a pure code-movement refactor that breaks up the three largest source files into module directories grouped by theme. There are no logic changes: the only edits that are not relocations are private to pub(crate) visibility bumps for items that are now referenced across modules or from tests, plus the re-export lines that keep every existing path working. No public API changes.
It follows the pattern that already exists in src/client/, where the root .rs file stays the module root and a sibling directory holds the per-theme submodules.
What moved
src/message.rs (12467 to 167 LOC at the root). The root keeps the helper structs, the free fns and the RetryReason re-export. The impl Client decrypt/receive pipeline is split into message/{dispatch, msg_secret, retry, receive, special}.rs, and the tests move to message/tests.rs.
src/client.rs (7117 to 810 LOC at the root). The root keeps the Client struct, the public types (NodeFilter, ClientError, MemoryDiagnostics) and the ack free fns. The 107 impl Client methods are split into client/{lifecycle, node_io, app_state, iq_ops, adapters, messaging, accessors}.rs, and the tests move to client/tests.rs. The pre-existing client submodules (device_registry, lid_pn, sessions, sender_keys, offline_resume, context_impl) are untouched.
wacore/src/send.rs (5322 to 84 LOC at the root). The root keeps mod stanza, StanzaType and the re-exports that preserve the wacore::send::* paths. The free fns are split into send/{classify, encrypt, dm, peer, group, status}.rs, and the tests move to send/tests.rs.
How paths stay intact
Each submodule is just
use super::*;plus the moved block, so it inherits the root module imports through the glob. impl methods are path independent and only needed visibility bumps to pub(crate). Free functions are re-exported from the root viapub use submod::*(and a couple ofpub(crate) uselines for the internal helpers that cross module boundaries), sowacore::send::prepare_dm_stanza,crate::message::RetryReasonand friends resolve exactly as before.Verification
cargo fmt --all applied. cargo clippy --all-targets -- -D warnings is clean. cargo test --workspace --exclude e2e-tests passes with 3975 passed and 0 failed. Test counts are preserved: 147 message, 73 client, 89 send, none lost in the move.
The test files are now in dedicated tests.rs modules out of the production files. They are still large (message/tests.rs is around 9.5k lines), so splitting them further by topic is a possible follow-up that I left out of this PR to keep it a clean, low-risk move.