fix(receive): skip PDO placeholder-resend for view-once, ack instead - #934
Conversation
A message that arrives with only an `<unavailable type="view_once">` child (no `<enc>`) currently triggers a PDO placeholder-resend to our own phone. But WhatsApp never fans view-once media out to companion/linked devices, so that request always comes back empty — the content is unrecoverable by design. Worse, the peer round-trip is surfaced by the phone as a spurious "Finished syncing with WhatsApp on <device>" notification, once for every view-once message the companion receives. Skip the PDO for `UnavailableType::ViewOnce`: still dispatch the `UndecryptableMessage` event (consumers keep seeing the failure) and still send the transport ack directly (previously the ack was gated on the PDO going out), so the offline queue drains and the stanza isn't redelivered. `Unknown` unavailables keep the existing PDO recovery path unchanged. Adds `view_once_stub_acks_without_pdo` covering the new behavior via the capturing-transport harness (event fires + `<ack class="message">` sent).
📝 WalkthroughWalkthroughAlright, this one's actually pretty clean. ChangesView-once ack without PDO
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant classify_incoming_message
participant EventDispatcher
participant Transport
Server->>classify_incoming_message: unavailable type=view_once stanza
classify_incoming_message->>classify_incoming_message: compute is_view_once
classify_incoming_message->>EventDispatcher: dispatch_undecryptable_event(ViewOnce, Show)
alt is_view_once true
classify_incoming_message->>Transport: send ack (skip PDO)
else other unavailable_type
classify_incoming_message->>Server: run_pdo_request
Server-->>classify_incoming_message: PDO sent status
classify_incoming_message->>Transport: send ack if PDO sent
end
Possibly related PRs
We need this to work flawlessly — acking without waiting on a dead-end PDO request is the right call. Ship it, but let's keep an eye on that 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/message/receive.rs`:
- Around line 104-107: The `matches!` call in `receive.rs` is moving
`unavailable_type`, which breaks later uses in the same flow. Update the
`is_view_once` check in the receive logic to match on a reference to
`unavailable_type` (or otherwise avoid consuming it) so the subsequent log
statement and `dispatch_undecryptable_event` call can still use the value.
In `@src/message/tests.rs`:
- Around line 5263-5275: The current test only waits for find_message_ack and
does not verify that no PDO placeholder-resend was sent, so it can miss
regressions in the skip-PDO contract. Update the test around transport.sent()
and the ack polling to add a negative assertion that no frame matching the PDO
request shape was captured before/while the ack appears. Keep the existing ack
check, but also inspect the recorded frames in this view-once stub path to
ensure the PDO send path never runs.
- Line 5253: The test fixture in the stanza setup uses a routable-looking
phone-number JID, which should be replaced with an obviously fictitious sender
identifier. Update the call to build_unavailable_stanza in message/tests.rs to
use a reserved synthetic JID value instead of a real-looking WhatsApp number so
the test data stays clearly non-production and non-identifying.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8725aa3-5c3e-4ea0-a117-8c8387c3b197
📒 Files selected for processing (2)
src/message/receive.rssrc/message/tests.rs
| let is_view_once = matches!( | ||
| unavailable_type, | ||
| crate::types::events::UnavailableType::ViewOnce | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the derives on UnavailableType and whether by-value matches! is safe here.
rg -n -C4 'pub enum UnavailableType|derive\(.*WireEnum|derive\(.*Copy' wacore/src/types/events.rs src/message/receive.rsRepository: oxidezap/whatsapp-rust
Length of output: 4499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant slice in src/message/receive.rs and the enum definition.
sed -n '90,150p' src/message/receive.rs
printf '\n---\n'
sed -n '979,1005p' wacore/src/types/events.rsRepository: oxidezap/whatsapp-rust
Length of output: 4068
Avoid moving unavailable_type here
matches! takes this enum by value, so the later log and dispatch_undecryptable_event use won’t compile. Match on &unavailable_type instead, or clone if you really need ownership.
🤖 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 104 - 107, The `matches!` call in
`receive.rs` is moving `unavailable_type`, which breaks later uses in the same
flow. Update the `is_view_once` check in the receive logic to match on a
reference to `unavailable_type` (or otherwise avoid consuming it) so the
subsequent log statement and `dispatch_undecryptable_event` call can still use
the value.
| // The stanza is acked so the offline queue drains, without gating on a PDO. | ||
| let mut acked = false; | ||
| for _ in 0..80 { | ||
| if find_message_ack(&transport.sent()).is_some() { | ||
| acked = true; | ||
| break; | ||
| } | ||
| tokio::time::sleep(std::time::Duration::from_millis(25)).await; | ||
| } | ||
| assert!( | ||
| acked, | ||
| "view-once stub must emit an <ack class=\"message\"> to drain the offline queue", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that PDO was not sent.
This only proves an ack eventually appears. A regression that sends the PDO placeholder-resend and then acks would still pass, so the test does not protect the core skip-PDO contract. Add a negative assertion over captured frames for the PDO request shape.
🤖 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/tests.rs` around lines 5263 - 5275, The current test only waits
for find_message_ack and does not verify that no PDO placeholder-resend was
sent, so it can miss regressions in the skip-PDO contract. Update the test
around transport.sent() and the ack polling to add a negative assertion that no
frame matching the PDO request shape was captured before/while the ack appears.
Keep the existing ack check, but also inspect the recorded frames in this
view-once stub path to ensure the PDO send path never runs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/message/tests.rs">
<violation number="1" location="src/message/tests.rs:5264">
P2: This test only proves an ack is eventually sent; it does not verify that PDO is skipped. Add a negative assertion over captured outbound frames so a PDO-then-ack regression fails.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -5218,8 +5218,9 @@ async fn test_unavailable_with_enc_skips_unavailable_shortcut() { | |||
|
|
|||
There was a problem hiding this comment.
P2: This test only proves an ack is eventually sent; it does not verify that PDO is skipped. Add a negative assertion over captured outbound frames so a PDO-then-ack regression fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/message/tests.rs, line 5264:
<comment>This test only proves an ack is eventually sent; it does not verify that PDO is skipped. Add a negative assertion over captured outbound frames so a PDO-then-ack regression fails.</comment>
<file context>
@@ -5236,6 +5237,44 @@ async fn test_unavailable_without_enc_dispatches_view_once_event() {
+ );
+
+ // The stanza is acked so the offline queue drains, without gating on a PDO.
+ let mut acked = false;
+ for _ in 0..80 {
+ if find_message_ack(&transport.sent()).is_some() {
</file context>
|
Thanks @blaueeiner <3 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
2 participants
Problem
A message that arrives with only an
<unavailable type="view_once">child (no<enc>) currently triggers a PDO placeholder-resend to our own phone (run_pdo_request). But WhatsApp never fans view-once media out to companion/linked devices, so that request always comes back empty — the content is unrecoverable by design (the code already documents this expectation elsewhere).Worse, the peer round-trip is surfaced by the phone as a spurious "Finished syncing with WhatsApp on <device>" notification — once for every view-once message the companion receives. This is a visible, repeated annoyance for anyone running a linked/companion client.
Fix
In the
<unavailable>-child branch ofhandle_incoming_message, when the type isViewOnce:run_pdo_requestentirely (it can only return empty and only causes the notification).UndecryptableMessageevent, so consumers keep seeing the failure.pdo_sent); with the PDO skipped we ack unconditionally (outside status-broadcast) so the offline queue drains and the server does not redeliver the stanza.Unknownunavailables keep the existing PDO recovery path completely unchanged — only view-once, which is provably unrecoverable, is short-circuited.Test
Adds
view_once_stub_acks_without_pdo, which drives a bare view-once stub throughhandle_incoming_messageon the capturing-transport harness and asserts both that theViewOnceUndecryptableMessagestill fires and that an<ack class="message">is sent. Also updates the now-stale comment ontest_unavailable_without_enc_dispatches_view_once_event(it claimed view-once "relays via PDO").Validation
cargo test -p whatsapp-rust --lib— new test passes; the full view-once/PDO/unavailable suite (24 tests) is green, no regressions.cargo fmt --checkclean.cargo clippy -p whatsapp-rust --lib -- -D warningsclean.Rebased on current
main.