fix(pdo): align PDO recovery + UndecryptableMessage dispatch with WA Web - #585
Conversation
Audit of a 45h production log (2026-04-21..23) turned up 17 messages lost to UndecryptableMessage after the decrypt pipeline exhausted its retry budget. Cross-checking against the captured WA Web JS in docs/captured-js/ exposed four places where we diverged from WAWebNon MessageDataRequestPlaceholderMessageResendUtils and from WAWebMessageProcessPlaceholder. Fixes * Drop the unconditional `server == Broadcast` short-circuit in spawn_pdo_request_with_options. Status posts (15/17 of the lost messages in the log were status@broadcast) and user broadcast lists now flow through PDO. Matches WAWebNonMessageDataRequestPlaceholder MessageResendUtils which excludes bot/hosted/view_once fanouts but not broadcast chats, and the response handler which has an explicit OTHER_STATUS branch. * Drop the `is_from_me` early return in spawn_pdo_request_with_options. When the user's primary phone sends a message, the fanout copy delivered to this client can still fail to decrypt; PDO is the only recovery path for that case. WAWebE2EProtoUtils.msgKeyToProtobuf omits the participant field when fromMe but does not skip the request itself. * Gate participant on `!is_from_me && (is_group || broadcast)`, which mirrors the participant logic in msgKeyToProtobuf. Groups and broadcasts need it so the phone can locate the stored message. * Skip PDO for messages older than 14 days, matching the `placeholder_message_resend_maximum_days_limit` AB prop (default 14) enforced by handlePlaceholderMsgsSeen. Prevents offline-sync or long-reconnect tails from flooding the phone with resend requests for messages the user likely no longer cares about. * Dedup UndecryptableMessage by (chat, msg_id) via a new 5m/1000-entry cache. A server resend of the same id after our retry receipt used to surface a duplicate event (msg 3AD01881AA95F7D81070 in the log fired twice, 28 lines apart). Matches WA Web's DB-level placeholder uniqueness in WAWebMessageProcessPlaceholder, which stores a single row per msg id and is idempotent on re-insert. Tests Each fix has a regression test that either inverts a pre-existing "this is a gap" test or covers a new code path: * test_pdo_armed_for_status_broadcast * test_pdo_armed_for_any_broadcast_chat * test_pdo_armed_for_from_me * test_pdo_skipped_for_ancient_messages * test_pdo_armed_for_one_on_one (sanity) * test_undecryptable_fires_before_retry_task (dispatch ordering) * test_undecryptable_deduped_across_resends (dedup) Validation: cargo fmt, cargo clippy --workspace --all-targets --exclude e2e-tests, cargo test --workspace --exclude e2e-tests (1263 tests passing, zero failures, zero clippy warnings).
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds a TTL-bounded cache and single‑flight dedupe for UndecryptableMessage dispatches, makes decrypt-failure handling async and awaited across call sites, exposes the cache in debug memory diagnostics, and changes PDO resend eligibility to a 14‑day age rule while preserving participant sender reconstruction. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(0, 123, 255, 0.5)
participant Msg as MessageProcessor
end
rect rgba(40, 167, 69, 0.5)
participant Client as Client
end
rect rgba(255, 193, 7, 0.5)
participant Cache as UndecryptableCache
end
rect rgba(108, 117, 125, 0.5)
participant Dispatcher as EventDispatcher
end
rect rgba(23, 162, 184, 0.5)
participant Retry as RetrySpawner
end
Msg->>Client: attempt decrypt -> failure
Client->>Cache: lookup (ChatMessageId)
alt cache miss
Cache-->>Client: miss
Client->>Dispatcher: dispatch UndecryptableMessage (first caller)
Client->>Cache: insert key (TTL)
else cache hit
Cache-->>Client: hit
Client->>Dispatcher: log duplicate, skip dispatch
end
Client->>Retry: spawn retry_receipt (always)
Retry-->>Client: scheduled retry task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 9s —— View job Review: fix(pdo): align PDO recovery + UndecryptableMessage dispatch with WA Web
Overall this is a well-motivated fix — good production evidence, clear WA Web cross-references, and solid regression tests. The changes are correct in direction and I'd approve with one issue addressed and a few nits considered. IssueDedup bypass on three direct The dedup cache (
If a server resend hits any of these three paths, the user would still see a duplicate Consider either routing these through Nits1. Display alignment off by one space — " undecryptable_dispatched:{}"Every other line uses padding to align the value column (e.g. 2. Non-atomic dedup is fine but the comment could be tighter — The comment says "two simultaneous racers both see 3. PDO age check uses The rest of the codebase uses 4. Test The 150ms sleep to verify the retry task has run is inherently racy under CI load. It passes today but could flake. Consider using a What looks good
|
![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: 5ba940b3e4
ℹ️ 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".
| // `fromMe` is NOT excluded here: when the user's other devices send a | ||
| // message and the fanout copy to this client fails to decrypt, PDO is | ||
| // the only recovery path. Matches WAWebNonMessageDataRequestPlaceholderMessageResendUtils. |
There was a problem hiding this comment.
Allowing fromMe messages into spawn_pdo_request_with_options makes a latent keying mismatch user-visible for LID DMs: send_pdo_placeholder_resend_request still derives the pending key from sender_alt when chat.is_lid(), but for self-sent DMs sender_alt is the account’s own PN (see wacore::messages::parse_message_info), while PDO responses are removed by remote_jid (the recipient chat JID). In this path the pending entry cannot be matched, so recovery falls back to reconstructed MessageInfo with PN chat/sender and leaves stale pending entries until TTL, which can surface recovered messages in the wrong chat identity and break dedup behavior for that message.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client.rs`:
- Around line 411-415: The current pattern around undecryptable_dispatched
(Cache<ChatMessageId, ()>) does a get() then insert() in the consumer (the
decrypt-failure path in message.rs), which allows two concurrent tasks to both
observe a miss and both dispatch; change the logic so the first write is the
decision point: replace the get-then-insert sequence with an atomic
"insert-if-absent" operation (e.g.
Cache::entry/insert_if_absent/get_or_insert_with or a compare-and-set style API)
so only the task that wins the insert proceeds to dispatch the placeholder; if
the Cache implementation lacks such API, guard a single insert via a per-id lock
or upgrade the Cache type to one that supports entry semantics and use
undecryptable_dispatched.entry(id).or_insert(()) (or equivalent) to detect
whether you actually inserted before dispatching.
In `@src/message.rs`:
- Around line 5302-5366: These tests call spawn_pdo_request_with_options(...)
then only sleep, so they don't verify PDO was actually armed; modify each test
(test_pdo_armed_for_status_broadcast, test_pdo_armed_for_any_broadcast_chat,
test_pdo_armed_for_one_on_one, test_pdo_armed_for_from_me) to assert that the
client's pdo_pending_requests contains the ChatMessageId for the message (use
client.pdo_pending_requests.get(&ChatMessageId::new(...)).await.is_some() or
equivalent) after calling spawn_pdo_request_with_options and the short sleep,
ensuring the positive arming side-effect is validated rather than relying solely
on timing.
- Around line 163-190: The current dedupe in handle_decrypt_failure still allows
duplicates because other code paths call dispatch_undecryptable_event directly
(e.g., the session-only fallback in process_classified_message and the
NoSenderKeyState group path); change those direct calls to route through the
same deduping entrypoint by invoking handle_decrypt_failure (or a shared helper
that performs the dedupe + insert + dispatch and then spawn_retry_receipt)
instead of calling dispatch_undecryptable_event directly, ensuring every path
(including the session-only fallback and NoSenderKeyState) uses the
undecryptable_dispatched cache and spawn_retry_receipt logic so only one
UndecryptableMessage per (chat, msg_id) is emitted.
In `@src/pdo.rs`:
- Around line 78-85: The fallback reconstruction path must use the participant
field for broadcast/status messages like it does for groups: update
message_info_from_web_message_info so it reads key.participant not only when
key.is_group is true but also when key.chat.server ==
wacore_binary::Server::Broadcast (or when key.participant.is_some()), ensuring
recovered broadcast PDO responses get the real author instead of the remote_jid;
adjust the logic that handles pdo_pending_requests fallback to prefer
key.participant when present (use the function name
message_info_from_web_message_info and the symbols key.participant,
wacore_binary::Server::Broadcast, and pdo_pending_requests to locate and change
the condition).
- Around line 474-476: The age check using age.num_days() is too coarse and
allows timestamps up to 14 days + 23h to pass; update the comparison in the
block that computes age =
chrono::Utc::now().signed_duration_since(info.timestamp) (with PDO_MAX_AGE_DAYS
defined) to compare the full Duration directly, e.g. replace the age.num_days()
> PDO_MAX_AGE_DAYS test with age > chrono::Duration::days(PDO_MAX_AGE_DAYS) so
messages are rejected at exactly 14 days.
🪄 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: f0630fee-2da1-4535-92a1-68aaa68059bd
📒 Files selected for processing (4)
src/cache_config.rssrc/client.rssrc/message.rssrc/pdo.rs
| async fn test_pdo_armed_for_status_broadcast() { | ||
| let client = create_test_client_for_retry_with_id("pdo_status").await; | ||
|
|
||
| let info = Arc::new(create_test_message_info( | ||
| "status@broadcast", | ||
| "STATUS_MSG_1", | ||
| "5511777776666@s.whatsapp.net", | ||
| )); | ||
|
|
||
| assert_eq!(info.source.chat.server, wacore_binary::Server::Broadcast); | ||
|
|
||
| client.spawn_pdo_request_with_options(&info, true); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
| } | ||
|
|
||
| /// Broadcast lists share the same code path; locks the guard for both. | ||
| #[tokio::test] | ||
| async fn test_pdo_armed_for_any_broadcast_chat() { | ||
| let client = create_test_client_for_retry_with_id("pdo_bcast_list").await; | ||
|
|
||
| let info = Arc::new(create_test_message_info( | ||
| "12345@broadcast", | ||
| "BCAST_LIST_MSG_1", | ||
| "5511777776666@s.whatsapp.net", | ||
| )); | ||
|
|
||
| assert_eq!(info.source.chat.server, wacore_binary::Server::Broadcast); | ||
|
|
||
| client.spawn_pdo_request_with_options(&info, true); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_pdo_armed_for_one_on_one() { | ||
| let client = create_test_client_for_retry_with_id("pdo_dm").await; | ||
|
|
||
| let info = Arc::new(create_test_message_info( | ||
| "85010891714716@lid", | ||
| "DM_MSG_1", | ||
| "85010891714716@lid", | ||
| )); | ||
|
|
||
| assert_ne!(info.source.chat.server, wacore_binary::Server::Broadcast); | ||
|
|
||
| client.spawn_pdo_request_with_options(&info, true); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
| } | ||
|
|
||
| /// fromMe messages fanned out to a linked device can still fail decrypt | ||
| /// on the receiver side; PDO is the only recovery path for them. | ||
| #[tokio::test] | ||
| async fn test_pdo_armed_for_from_me() { | ||
| let client = create_test_client_for_retry_with_id("pdo_from_me").await; | ||
|
|
||
| let mut info = create_test_message_info( | ||
| "85010891714716@lid", | ||
| "FROM_ME_MSG_1", | ||
| "5511777776666@s.whatsapp.net", | ||
| ); | ||
| info.source.is_from_me = true; | ||
| let info = Arc::new(info); | ||
|
|
||
| client.spawn_pdo_request_with_options(&info, true); | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
| } |
There was a problem hiding this comment.
These PDO regression tests don’t lock the behavior.
Right now these tests only sleep after spawn_pdo_request_with_options(). If PDO arming regresses into a no-op again, they still pass. Please assert the positive side effect that test_pdo_skipped_for_ancient_messages already uses — e.g. client.pdo_pending_requests.get(&ChatMessageId::new(...)).await.is_some().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 5302 - 5366, These tests call
spawn_pdo_request_with_options(...) then only sleep, so they don't verify PDO
was actually armed; modify each test (test_pdo_armed_for_status_broadcast,
test_pdo_armed_for_any_broadcast_chat, test_pdo_armed_for_one_on_one,
test_pdo_armed_for_from_me) to assert that the client's pdo_pending_requests
contains the ChatMessageId for the message (use
client.pdo_pending_requests.get(&ChatMessageId::new(...)).await.is_some() or
equivalent) after calling spawn_pdo_request_with_options and the short sleep,
ensuring the positive arming side-effect is validated rather than relying solely
on timing.
Benchmark Results1 improvement(s):
66 unchanged benchmark(s)
|
`num_days()` truncates, so a message at age 14d1m reported 14 and passed our `> 14` check — WA Web's `age_s > 14 * DAY_SECONDS` would reject. Switched to a `Duration` comparison to close the up-to-24h leniency window. Also: aligned the new MemoryDiagnostics entry with the surrounding column and gave the fromMe regression test a realistic own-JID sender. Added a 14d+1m boundary test that catches the truncation regression.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2180d3c4e9
ℹ️ 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".
| // `fromMe` is NOT excluded here: when the user's other devices send a | ||
| // message and the fanout copy to this client fails to decrypt, PDO is | ||
| // the only recovery path. Matches WAWebNonMessageDataRequestPlaceholderMessageResendUtils. |
There was a problem hiding this comment.
Allowing fromMe messages through this path exposes a key mismatch in send_pdo_placeholder_resend_request: for LID DMs it still builds the pending key from sender_alt, which is the account’s own PN on self-sent messages, while handle_placeholder_resend_response removes by web_msg_info.key.remote_jid (the recipient chat). In that scenario the pending entry cannot be matched, so recovery falls back to reconstructed metadata and leaves stale pending entries until TTL, which can surface the recovered message under inconsistent identity/dedup behavior.
Useful? React with 👍 / 👎.
…aths
Two issues surfaced in review:
1. The dedup in handle_decrypt_failure used a non-atomic get+insert
pair on the undecryptable_dispatched cache, so two concurrent callers
could both observe a miss and both dispatch. Switched to moka's
single-flight `get_with` with an AtomicBool side-channel: only the
caller whose init runs sets the flag to true, so exactly one caller
dispatches per (chat, msg_id).
2. Three other paths called `dispatch_undecryptable_event` directly and
bypassed the dedup entirely:
- the session-only fallback in `process_classified_message`
- the `NoSenderKeyState` group branch
- the `<unavailable>` stanza pre-decrypt path
`dispatch_undecryptable_event` is now async and is the single
entrypoint that both checks and updates the dedup cache before
touching the event bus. The signature takes `is_unavailable` /
`unavailable_type` so the unavailable-stanza path keeps its
distinguishing fields while still going through the gate.
Added `test_undecryptable_dedup_is_atomic`: 32 concurrent callers for
the same id must collapse to exactly one event. Would fail under the
old get+insert pattern.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/pdo.rs (1)
78-84:⚠️ Potential issue | 🟡 MinorBroadcast PDO fallback still drops the real sender.
This request path now writes
participantfor broadcast/status chats, but ifpdo_pending_requestsmisses, Line 403 still only readskey.participantfor groups. The recovered message then falls back tosender = remote_jid(status@broadcast) instead of the actual author.Suggested fix
- let sender = if is_group { + let sender = if is_group + || remote_jid.server == wacore_binary::Server::Broadcast + || key.participant.is_some() + { key.participant .as_ref() .map(|p: &String| p.parse()) .transpose()? .unwrap_or_else(|| remote_jid.clone())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pdo.rs` around lines 78 - 84, When reconstructing recovered PDO messages in src/pdo.rs, the code currently only uses key.participant for groups and falls back to remote_jid (e.g., "status@broadcast") which drops the real sender for broadcast chats; modify the recovery path to, when key.participant is None and the chat server is Broadcast (wacore_binary::Server::Broadcast), attempt to look up the original participant from pdo_pending_requests (the same pending-request map used when writing participant earlier) and use that value as the sender/participant; only fall back to remote_jid if the pdo_pending_requests lookup fails.src/message.rs (2)
5302-5364:⚠️ Potential issue | 🟡 MinorThese PDO arming tests still don’t prove PDO was armed.
Sleeping after
spawn_pdo_request_with_options()doesn’t lock the behavior. If that call regresses to a no-op again, these four tests still pass. Assert the positive side effect like the age-based tests already do: build theChatMessageIdand verifyclient.pdo_pending_requests.get(&cache_key).await.is_some()for the status, broadcast-list, 1:1, andfrom_mecases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 5302 - 5364, The tests (test_pdo_armed_for_status_broadcast, test_pdo_armed_for_any_broadcast_chat, test_pdo_armed_for_one_on_one, test_pdo_armed_for_from_me) only sleep after calling spawn_pdo_request_with_options() and don’t actually assert PDO was armed; change each test to build the ChatMessageId for the created info (use the same key construction used by spawn_pdo_request_with_options, e.g. ChatMessageId::new or the project’s cache key helper) and then await client.pdo_pending_requests.get(&cache_key).await.is_some() (assert true) instead of relying on tokio::time::sleep; keep the call to spawn_pdo_request_with_options(&info, true) but replace the sleep with the explicit check to ensure the pending request entry exists.
163-190:⚠️ Potential issue | 🟠 MajorThis still doesn’t guarantee one placeholder per message.
The new helper isn’t the only gate yet.
dispatch_undecryptable_event()still fires directly on Line 676, Line 701, and Line 1208, so those paths bypass the cache entirely. And inside this helper, theget(...).await+insert(...).awaitpair is still racy, so two concurrent deliveries of the same(chat, msg_id)can both emit before either insert lands. That means the uniqueness invariant is still best-effort, not guaranteed. Please funnel every undecryptable emission through one atomic dedup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 163 - 190, Multiple code paths still call dispatch_undecryptable_event directly and the current get(...).await + insert(...).await pair in handle_decrypt_failure is racy, so enforce a single atomic dedup path by creating a single helper (e.g., emit_undecryptable_once or make dispatch_undecryptable_event private and wrap it) that performs an atomic check-and-insert against undecryptable_dispatched and only when that atomic operation returns "first" does it call dispatch_undecryptable_event; replace the get+insert sequence in handle_decrypt_failure with that atomic call, and change all other direct calls (the ones at the prior locations that call dispatch_undecryptable_event) to call the new helper so every undecryptable emission goes through the same atomic dedup logic; if the current cache type lacks an atomic entry API, use an async mutex around the map or add an insert_if_absent method on undecryptable_dispatched that returns whether the key was newly inserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/message.rs`:
- Around line 5302-5364: The tests (test_pdo_armed_for_status_broadcast,
test_pdo_armed_for_any_broadcast_chat, test_pdo_armed_for_one_on_one,
test_pdo_armed_for_from_me) only sleep after calling
spawn_pdo_request_with_options() and don’t actually assert PDO was armed; change
each test to build the ChatMessageId for the created info (use the same key
construction used by spawn_pdo_request_with_options, e.g. ChatMessageId::new or
the project’s cache key helper) and then await
client.pdo_pending_requests.get(&cache_key).await.is_some() (assert true)
instead of relying on tokio::time::sleep; keep the call to
spawn_pdo_request_with_options(&info, true) but replace the sleep with the
explicit check to ensure the pending request entry exists.
- Around line 163-190: Multiple code paths still call
dispatch_undecryptable_event directly and the current get(...).await +
insert(...).await pair in handle_decrypt_failure is racy, so enforce a single
atomic dedup path by creating a single helper (e.g., emit_undecryptable_once or
make dispatch_undecryptable_event private and wrap it) that performs an atomic
check-and-insert against undecryptable_dispatched and only when that atomic
operation returns "first" does it call dispatch_undecryptable_event; replace the
get+insert sequence in handle_decrypt_failure with that atomic call, and change
all other direct calls (the ones at the prior locations that call
dispatch_undecryptable_event) to call the new helper so every undecryptable
emission goes through the same atomic dedup logic; if the current cache type
lacks an atomic entry API, use an async mutex around the map or add an
insert_if_absent method on undecryptable_dispatched that returns whether the key
was newly inserted.
In `@src/pdo.rs`:
- Around line 78-84: When reconstructing recovered PDO messages in src/pdo.rs,
the code currently only uses key.participant for groups and falls back to
remote_jid (e.g., "status@broadcast") which drops the real sender for broadcast
chats; modify the recovery path to, when key.participant is None and the chat
server is Broadcast (wacore_binary::Server::Broadcast), attempt to look up the
original participant from pdo_pending_requests (the same pending-request map
used when writing participant earlier) and use that value as the
sender/participant; only fall back to remote_jid if the pdo_pending_requests
lookup fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1220772b-497a-4198-a04e-2affc80b81c8
📒 Files selected for processing (3)
src/client.rssrc/message.rssrc/pdo.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e27aa760b
ℹ️ 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".
| // `fromMe` is NOT excluded here: when the user's other devices send a | ||
| // message and the fanout copy to this client fails to decrypt, PDO is | ||
| // the only recovery path. Matches WAWebNonMessageDataRequestPlaceholderMessageResendUtils. |
There was a problem hiding this comment.
Allowing fromMe messages down this path exposes a pending-key mismatch for self-sent LID DMs: send_pdo_placeholder_resend_request still keys pdo_pending_requests from sender_alt (own PN in this case), but handle_placeholder_resend_response removes entries by WebMessageInfo.key.remote_jid (the recipient chat JID). When these differ, the pending entry is never matched, so recovery falls back to reconstructed metadata and leaves stale pending rows until TTL, which can surface recovered content under inconsistent identity/dedup behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/message.rs`:
- Around line 5476-5498: The test
test_undecryptable_event_has_no_pending_pdo_hint is only checking struct field
access for UndecryptableMessage at compile time to catch accidental additions
like a pending_pdo flag; replace this runtime-access style test with an explicit
compile-time assertion: change the test to use a zero-sized, compile-time check
(e.g., a const assertion or a type-level check) that verifies the shape of
UndecryptableMessage (ensuring fields info, is_unavailable, unavailable_type,
decrypt_fail_mode remain present and no unexpected public fields such as
pending_pdo exist) so maintainers get a clear compile-time failure; reference
the symbols UndecryptableMessage,
test_undecryptable_event_has_no_pending_pdo_hint, create_test_message_info,
UnavailableType, and DecryptFailMode when locating and updating the test.
- Around line 186-201: Update the doc/comment for handle_decrypt_failure to
clarify return semantics: state that the function returns true to indicate the
decrypt failure was handled (for assigning to the dispatched_undecryptable flag)
even if dispatch_undecryptable_event was suppressed as a duplicate, rather than
implying the event was necessarily dispatched; reference the function name
handle_decrypt_failure, the called dispatch_undecryptable_event, and the
dispatched_undecryptable flag so readers understand the distinction.
🪄 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: 43d73515-a75b-414c-ac45-f219628f9f2f
📒 Files selected for processing (1)
src/message.rs
| async fn handle_decrypt_failure( | ||
| self: &Arc<Self>, | ||
| info: &Arc<MessageInfo>, | ||
| reason: RetryReason, | ||
| decrypt_fail_mode: crate::types::events::DecryptFailMode, | ||
| ) -> bool { | ||
| self.dispatch_undecryptable_event(Arc::clone(info), decrypt_fail_mode); | ||
| self.dispatch_undecryptable_event( | ||
| Arc::clone(info), | ||
| false, | ||
| crate::types::events::UnavailableType::Unknown, | ||
| decrypt_fail_mode, | ||
| ) | ||
| .await; | ||
| self.spawn_retry_receipt(info, reason); | ||
| true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The return value semantics could be clearer, but the design is sound.
handle_decrypt_failure now discards the return value of dispatch_undecryptable_event and always returns true. This works because:
- The cache provides cross-call dedup anyway
- The
dispatched_undecryptableflag is used to skip redundant calls within the same processing flow, not to track actual dispatch
However, the comment at line 185 says "Returns true to be assigned to dispatched_undecryptable flag" — this is slightly misleading since it returns true even when dispatch was suppressed as duplicate. Consider updating the comment to clarify it means "failure was handled" rather than "event was dispatched."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 186 - 201, Update the doc/comment for
handle_decrypt_failure to clarify return semantics: state that the function
returns true to indicate the decrypt failure was handled (for assigning to the
dispatched_undecryptable flag) even if dispatch_undecryptable_event was
suppressed as a duplicate, rather than implying the event was necessarily
dispatched; reference the function name handle_decrypt_failure, the called
dispatch_undecryptable_event, and the dispatched_undecryptable flag so readers
understand the distinction.
| #[test] | ||
| fn test_undecryptable_event_has_no_pending_pdo_hint() { | ||
| use crate::types::events::{UnavailableType, UndecryptableMessage}; | ||
|
|
||
| let info = Arc::new(create_test_message_info( | ||
| "5511999998888@s.whatsapp.net", | ||
| "SHAPE_MSG", | ||
| "5511777776666@s.whatsapp.net", | ||
| )); | ||
| let event = UndecryptableMessage { | ||
| info, | ||
| is_unavailable: false, | ||
| unavailable_type: UnavailableType::Unknown, | ||
| decrypt_fail_mode: DecryptFailMode::Show, | ||
| }; | ||
|
|
||
| let _ = ( | ||
| &event.info, | ||
| &event.is_unavailable, | ||
| &event.unavailable_type, | ||
| &event.decrypt_fail_mode, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
This test is more of a compile-time struct shape check than a behavioral test.
It accesses all fields of UndecryptableMessage to ensure the struct hasn't changed unexpectedly. The comment says it forces "a conscious UX decision" if someone adds a pending_pdo flag — that's reasonable as a regression guard, though it would be clearer as a compile-time assertion using #[forbid(dead_code)] or similar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 5476 - 5498, The test
test_undecryptable_event_has_no_pending_pdo_hint is only checking struct field
access for UndecryptableMessage at compile time to catch accidental additions
like a pending_pdo flag; replace this runtime-access style test with an explicit
compile-time assertion: change the test to use a zero-sized, compile-time check
(e.g., a const assertion or a type-level check) that verifies the shape of
UndecryptableMessage (ensuring fields info, is_unavailable, unavailable_type,
decrypt_fail_mode remain present and no unexpected public fields such as
pending_pdo exist) so maintainers get a clear compile-time failure; reference
the symbols UndecryptableMessage,
test_undecryptable_event_has_no_pending_pdo_hint, create_test_message_info,
UnavailableType, and DecryptFailMode when locating and updating the test.
`message_info_from_web_message_info` only honoured `key.participant` for groups. For broadcast responses — including status — it fell through to using `remote_jid` as the sender, so recovered status messages surfaced with sender=`status@broadcast` and the real author was lost. Matches the response handler in WAWebNonMessageDataRequestHandlerPlaceholderResend, which maps `participant` to `author` for both broadcast branches. The logic is now: prefer `key.participant` whenever present, fall back to own PN for fromMe, and only fall back to `remote_jid` for DMs. Added two reconstruction tests: * `test_reconstruct_prefers_participant_for_status_broadcast` — status broadcast response with participant set must surface the real author. * `test_reconstruct_dm_falls_back_to_remote_jid` — DM without participant still resolves sender to the chat peer.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pdo.rs`:
- Around line 574-655: Both tests
(test_reconstruct_prefers_participant_for_status_broadcast and
test_reconstruct_dm_falls_back_to_remote_jid) duplicate backend/pm/client setup;
extract that into a shared async helper (e.g. setup_test_client or
create_test_backend_and_client) that creates create_test_backend().await,
PersistenceManager::new(...).await.unwrap(), and Client::new(...).await and
returns the Client (and any rx/pm needed); then replace the duplicated blocks in
both tests with a call to that helper and use its returned values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d36dd203b4
ℹ️ 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".
| let dedup_key = | ||
| wacore::types::message::ChatMessageId::new(info.source.chat.clone(), info.id.clone()); |
There was a problem hiding this comment.
Build the undecryptable_dispatched key from a normalized chat JID (e.g., via resolve_encryption_jid) instead of raw info.source.chat. The same logical DM can arrive as PN on one delivery and LID on a resend during migration, and parse_message_info preserves whichever form was on the stanza; with the current key this produces two different cache entries and emits duplicate UndecryptableMessage events for one message ID, which defeats the dedup behavior this change is introducing.
Useful? React with 👍 / 👎.
…aths
Four tests in `pdo::tests` now exercise `message_info_from_web_message_info`
(status broadcast, DM without participant, LID-migrated DM, fromMe LID DM)
and share the same client-with-mock-transport bring-up. Factored that into
`setup_reconstruct_client` and `make_web_msg` helpers to cut ~50 lines of
repeated boilerplate.
The two new tests cover the WA-Web side-branch where a LID-migrated 1-on-1
response arrives with no participant and a LID `remote_jid`. Matches what
`WAWebSendNonMessageDataRequest` builds when the chat went through
`toUserLid` + fresh `MsgKey({id, fromMe, remote})` construction.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e4e710b45
ℹ️ 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".
| // `fromMe` is NOT excluded here: when the user's other devices send a | ||
| // message and the fanout copy to this client fails to decrypt, PDO is | ||
| // the only recovery path. Matches WAWebNonMessageDataRequestPlaceholderMessageResendUtils. |
There was a problem hiding this comment.
Removing the fromMe guard here routes self-sent LID DMs into PDO, but pending keys are still derived from sender_alt in send_pdo_placeholder_resend_request (own PN in this path) while responses are removed by WebMessageInfo.key.remote_jid (the recipient chat). That mismatch makes pdo_pending_requests.remove(...) miss, so recovery falls back to reconstructed metadata and leaves stale pending entries until TTL, which can surface identity/dedup inconsistencies for those messages. Fresh evidence: this commit enables the previously blocked path by dropping the is_from_me early return without updating the LID-DM pending-key strategy.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Audit of a 45-hour production log surfaced 17 messages permanently
lost to
UndecryptableMessageafter the retry pipeline gave up.Comparing each loss against the captured WA Web JS in
docs/captured-js/exposed four places where our PDO recovery path diverged from
WAWebNonMessageDataRequestPlaceholderMessageResendUtils/WAWebMessageProcessPlaceholder.This PR aligns the divergences. 15 of the 17 losses would have been
recovered by fix #1 alone.
Changes
server == Broadcastshort-circuit fromspawn_pdo_request_with_optionsOTHER_STATUSbranchis_from_meearly return fromspawn_pdo_request_with_optionsparticipantfield, still sends the requestparticipanton!is_from_me && (is_group || broadcast)msgKeyToProtobuf: omit when fromMe or DM, include otherwiseplaceholder_message_resend_maximum_days_limitAB prop (default 14) enforced inhandlePlaceholderMsgsSeenUndecryptableMessagedispatch by(chat, msg_id)WAWebMessageProcessPlaceholdermeans the UI sees one placeholder per idThe dedup mechanism is different from WA Web by necessity (we have no
DB placeholder model) but the observable behavior (one notification per
msg id) is equivalent.
Production log evidence
status@broadcast— first fix alone recoversthem.
3AD01881AA95F7D81070(a 1-on-1 msg) firedUndecryptableMessagetwice (lines 95089 and 95117), one second apart; dedup fixes this.
does not affect the historical losses but prevents future
offline-sync storms.
Test plan
cargo fmt --allcargo clippy --workspace --all-targets --exclude e2e-tests(zero warnings)cargo test --workspace --exclude e2e-tests(1263 passing, 0 failures)test_pdo_armed_for_status_broadcasttest_pdo_armed_for_any_broadcast_chattest_pdo_armed_for_from_metest_pdo_skipped_for_ancient_messagestest_pdo_armed_for_one_on_one(sanity for the DM path)test_undecryptable_fires_before_retry_task(dispatch ordering)test_undecryptable_deduped_across_resends(dedup)