Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions src/message/receive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,31 @@ impl Client {
Some("view_once") => crate::types::events::UnavailableType::ViewOnce,
_ => crate::types::events::UnavailableType::Unknown,
};
log::info!(
"[msg:{}] Message has <unavailable> child (type: {:?}), requesting from phone via PDO",
info.id,
unavailable_type
// View-once media is never fanned out to companion/linked devices,
// so a PDO placeholder-resend to our own phone always comes back
// empty — the content is unrecoverable by design. Worse, that peer
// round-trip is surfaced by the phone as a spurious "Finished
// syncing with WhatsApp on <device>" notification for every
// view-once message received. Skip the PDO for view-once; still
// dispatch the event (so consumers see the failure) and still ack
// so the offline queue is cleared and the stanza is not redelivered.
let is_view_once = matches!(
unavailable_type,
crate::types::events::UnavailableType::ViewOnce
);
Comment on lines +104 to 107

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.rs

Repository: 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.rs

Repository: 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.

if is_view_once {
log::info!(
"[msg:{}] Message has <unavailable> child (type: ViewOnce); \
unrecoverable via PDO — skipping request and acking",
info.id,
);
} else {
log::info!(
"[msg:{}] Message has <unavailable> child (type: {:?}), requesting from phone via PDO",
info.id,
unavailable_type
);
}
// PDO is the only recovery here (no retry receipt), so run it before
// the transport ack in one flush task: the ack must not clear the
// offline queue before the PDO request goes out. status is acked by
Expand All @@ -113,10 +133,16 @@ impl Client {
let info2 = Arc::clone(&info);
let skip_ack = info.source.chat.is_status_broadcast();
self.outbound_flush.spawn(&*self.runtime, async move {
// Only ack once the PDO request is out (or skipped as ancient);
// a transient send failure leaves it queued for redelivery.
let pdo_sent = client.run_pdo_request(&info2).await;
if !skip_ack && pdo_sent {
// View-once has no recoverable content, so skip the PDO entirely
// and ack directly. Otherwise PDO is the only recovery: ack only
// once the request is out (or skipped as ancient); a transient
// send failure leaves it queued for redelivery.
let should_ack = if is_view_once {
true
} else {
client.run_pdo_request(&info2).await
};
if !skip_ack && should_ack {
client.send_transport_ack(&info2).await;
}
});
Expand Down
43 changes: 41 additions & 2 deletions src/message/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5218,8 +5218,9 @@ async fn test_unavailable_with_enc_skips_unavailable_shortcut() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

/// Untrusted companions (web-class `PlatformType`) get the bare stub —
/// `<unavailable>` without `<enc>`. That path must still emit a
/// `ViewOnce` `UndecryptableMessage` so consumers surface the failure
/// while the phone relays via PDO.
/// `ViewOnce` `UndecryptableMessage` so consumers surface the failure.
/// (The PDO resend is deliberately skipped for view-once — see
/// `view_once_stub_acks_without_pdo`.)
#[tokio::test]
async fn test_unavailable_without_enc_dispatches_view_once_event() {
let client = create_test_client_for_retry_with_id("unavailable_stub").await;
Expand All @@ -5236,6 +5237,44 @@ async fn test_unavailable_without_enc_dispatches_view_once_event() {
);
}

/// View-once media is never shared with companion/linked devices, so a PDO
/// placeholder-resend to our own phone comes back empty — and that peer
/// round-trip surfaces as a spurious "Finished syncing with WhatsApp on
/// <device>" notification on the user's phone for every view-once received.
/// The bare view-once stub must therefore ack the stanza directly (so the
/// server stops redelivering it) instead of gating the ack on a futile PDO
/// request, while still surfacing the failure to consumers.
#[tokio::test]
async fn view_once_stub_acks_without_pdo() {
let (client, transport) = capturing_client("view_once_ack").await;
let recorder = Arc::new(EventRecorder::default());
client.register_handler(recorder.clone());

let node = build_unavailable_stanza("5511777776666@s.whatsapp.net", "VIEW_ONCE_ACK_1", false);
Comment thread
jlucaso1 marked this conversation as resolved.
Comment thread
jlucaso1 marked this conversation as resolved.
client.clone().handle_incoming_message(node).await;

// The (unrecoverable) failure still surfaces to consumers.
assert_eq!(
recorder.view_once_unavailable_count(),
1,
"bare view-once stub must still dispatch a ViewOnce UndecryptableMessage",
);

// 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",
);
Comment on lines +5263 to +5275

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

}

/// The event struct has no "recovery pending" flag, so consumers cannot
/// wait for a PDO outcome before surfacing failure — adding a field
/// here forces a conscious UX decision.
Expand Down
Loading