fix(offline): transport-ack stanzas with only unrecognized enc types - #648
Merged
Merged
Conversation
A stanza whose enc nodes are all unknown to the client (e.g. msmsg from the Meta AI bot) used to silently fall through classify_incoming_message without any ack, so the server replayed it from the offline queue every reconnect until <stream:error> closed the stream. Track had_unknown_enc and had_custom_handler in the enc loop; when both session_payloads and group_payloads are empty, an unknown enc was seen, and no custom handler claimed any enc, spawn a transport ack and return None. status@broadcast is skipped here because should_ack already covers it. Custom-handler-only flows are left untouched because the handler is responsible for its own ack. Decryption of msmsg itself is a follow-up; this PR just stops the offline queue from looping on stanzas we cannot decrypt yet.
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDetects incoming stanzas whose ChangesUnknown-only encryption payload transport ack
Sequence Diagram(s)sequenceDiagram
participant Classifier
participant Client
participant OutboundFlush
participant Server
Classifier->>Classifier: detect had_unknown_enc && !had_custom_handler && no session/group payloads
Classifier->>Client: spawn_node_transport_ack(node_ref)
Client->>Client: encode_ack_bytes(node_ref, own_pn)
Client->>OutboundFlush: outbound_flush.spawn(encoded_ack_bytes)
OutboundFlush->>Server: send transport <ack class="message">
Server-->>Classifier: stanza removed/dropped from offline queue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a5a83cde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/message.rs (1)
572-622:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat known-but-empty
<enc>nodes like unknown-only traffic.
EncPayload::from_owned_node()returnsNonefor both unknown enc types and known encs with missing content. Right now both sethad_unknown_enc, so a stanza like<enc type="msg"/>takes the new transport-ack fallback and gets dropped as if it weremsmsg. Split that bookkeeping before the fallback.Suggested fix
let payload = match EncPayload::from_owned_node(node, enc_node) { Some(p) => p, None => { - log::warn!("Enc node has no content or unknown type: {enc_type}"); - had_unknown_enc = true; + if EncType::from_wire(enc_type.as_ref()).is_none() { + log::warn!("Enc node has unknown type: {enc_type}"); + had_unknown_enc = true; + } else { + log::warn!("Enc node has no content: {enc_type}"); + } continue; } };🤖 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.rs` around lines 572 - 622, The code treats both unknown enc types and known-but-empty <enc> nodes the same because EncPayload::from_owned_node(node, enc_node) returns None in both cases; update the match branch that currently sets had_unknown_enc to distinguish unknown enc types from known-but-empty content: use the enc_type value (or call EncType::from_wire_str / equivalent) to detect if the enc type is recognized and only set had_unknown_enc when the type is unrecognized, leaving known-but-empty enc nodes unflagged so session_payloads/group_payloads logic and the transport-ack fallback (which checks session_payloads, group_payloads, had_unknown_enc, had_custom_handler and calls self.spawn_message_ack(&info)) behave correctly.
🤖 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.
Outside diff comments:
In `@src/message.rs`:
- Around line 572-622: The code treats both unknown enc types and
known-but-empty <enc> nodes the same because EncPayload::from_owned_node(node,
enc_node) returns None in both cases; update the match branch that currently
sets had_unknown_enc to distinguish unknown enc types from known-but-empty
content: use the enc_type value (or call EncType::from_wire_str / equivalent) to
detect if the enc type is recognized and only set had_unknown_enc when the type
is unrecognized, leaving known-but-empty enc nodes unflagged so
session_payloads/group_payloads logic and the transport-ack fallback (which
checks session_payloads, group_payloads, had_unknown_enc, had_custom_handler and
calls self.spawn_message_ack(&info)) behave correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 801479d8-9dcf-47a9-b320-6c9394e7e2e6
📒 Files selected for processing (1)
src/message.rs
…epair e2e Codex P2: the unknown-enc fallback was reconstructing the ack from MessageInfo, but parse_message_info only retains `recipient` on the self-sent branch, so LID-routed / hosted-companion / peer stanzas lost the attribute and the server replied <stream:error><ack/>. Add `Client::spawn_node_transport_ack` which encodes from the original NodeRef so `recipient` is echoed verbatim, and use it in the classify fallback. Outside-diff comment: `EncPayload::from_owned_node` returns None for both unknown types and known-but-empty content. Resolve `EncType::from_wire` up front and only set `had_unknown_enc` when the type itself is unrecognized; known-but-empty (malformed) falls back through the regular flow. Repair the e2e regression introduced by PR #647: `with_push_name` pre-seeds the name, so the setting_pushName mutation arrives with `old == new` and skips the auto `presence().set_available()` that would flip send_active_receipts to 1. As a result delivery receipts went out as type="inactive" and `test_bidirectional_offline_receipt` / `test_deferred_delivery_receipt_on_reconnect` failed in CI. Force active receipts in `TestClient::connect_inner` (mirrors whatsmeow's `SetForceActiveDeliveryReceipts`). Tests: - `unknown_only_enc_ack_preserves_recipient` for the LID-routed ack shape. - `known_enc_type_with_empty_content_skips_fallback_ack` for the new gate.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/message.rs`:
- Around line 577-581: When EncPayload::from_owned_node(node, enc_node) returns
None for a known enc_type, don't just log and continue — mark the message as a
terminal malformed stanza so it will be handled by the malformed path. Replace
the current continue branch in the match by setting the flag used to indicate
non-processable encs (e.g., set had_unknown_enc / had_malformed_enc to true or
otherwise mark the message as malformed), stop further enc-node processing so
classification yields a malformed ClassifiedMessage, and let
process_classified_message emit the appropriate ack/nack; reference
EncPayload::from_owned_node, enc_node, enc_type, had_unknown_enc, and
ClassifiedMessage to locate the change.
In `@tests/e2e/src/lib.rs`:
- Around line 140-142: Replace the three-line explanatory comment above the call
that uses with_push_name with a single concise sentence stating: "with_push_name
pre-seeds the name so the setting_pushName mutation has old==new (skipping auto
set_available), so force active to keep delivery receipts from being
type='inactive'." Reference the same context (with_push_name and
setting_pushName) and remove the play‑by‑play lines.
- Around line 140-143: The current workaround unconditionally calls
client.set_force_active_delivery_receipts(true), forcing active receipts for all
e2e clients; change this to only set forced active receipts when the client was
created with a pre-seeded push name by checking the push_name Optional (i.e., if
push_name.is_some()) before calling set_force_active_delivery_receipts; locate
the client creation code path used by connect_without_push_name/connect_*
helpers and wrap the set_force_active_delivery_receipts(true) call in a
conditional that verifies push_name.is_some() so clients created with None keep
normal receipt behavior.
🪄 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: f48ddd06-8c94-4e38-af69-ee072357fecb
📒 Files selected for processing (3)
src/client.rssrc/message.rstests/e2e/src/lib.rs
- Known-type-with-empty content also produces no usable payload, so include it in had_unknown_enc. process_classified_message has no branch for both payload vectors empty, so without flagging the stanza would loop in the offline queue too. - Invert the known-but-empty test to assert the ack is now emitted. - e2e: trim the workaround comment and gate set_force_active_delivery_receipts on push_name.is_some() so connect_without_push_name keeps the natural setting_pushName-triggered set_available path.
|
Actionable comments posted: 0 |
This was referenced May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
msmsgfrom the Meta AI bot). Classify silently dropped them, so the server replayed them every reconnect until<stream:error>closed the stream.classify_incoming_message: when bothsession_payloadsandgroup_payloadsend up empty AND at least one enc was unknown AND no custom handler claimed any enc, spawn a transport<ack>to drop the stanza from the offline queue.status@broadcastis left alone (theshould_ackgate already covers it).msmsgitself is intentionally out of scope; a follow-up PR will implement it. This change just stops the offline queue from looping on stanzas we cannot decrypt yet.Test plan
cargo fmt --allcargo clippy --all --tests(clean)cargo test -p whatsapp-rust -p wacore -p waproto(all green)