fix(pdo): request a placeholder resend at most once per message - #841
Conversation
A stuck sender redelivering the same undecryptable message every ~11s for 3h made the capped retry path fire an immediate PDO per copy: 692 encrypted peer messages to our own phone, each answered without content. The pending cache only dedups in-flight requests, so it emptied as soon as the phone answered. Mirror WA Web's session-lifetime set (WAWebNonMessageDataRequestPlaceholderMessageResendUtils): a pdo_requested memo keyed by chat+id gates send_pdo_placeholder_resend_request, released only when the send itself fails. Same storm also ran the PN-to-LID migration retry decrypt per copy with nothing to migrate, logging a second identical decrypt failure each time; migrate_signal_sessions_on_lid_discovery now reports whether anything moved and the retry decrypt is skipped when it did not. Log parity with the WA Web decryption handler: the skmsg skip after a failed pkmsg is silent there (canDecryptNext), and a content-less placeholder response is telemetry only, so downgrade both.
|
Warning Review limit reached
More reviews will be available in 13 minutes and 3 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR wires placeholder-resend PDO request deduplication into the client through a new per-message cache, preventing redundant requests across server redeliveries. It makes the PN-to-LID signal migration observable by returning a boolean, allowing decrypt retries to short-circuit when no state actually moved. Configuration, initialization, and retry paths are updated coherently. ChangesPDO Request Deduplication with Migration State Observable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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: 9d1c2ace6b
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pdo.rs (1)
101-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAt-most-once PDO is still race-prone under concurrent calls.
This sequence is non-atomic (
get→get→insert), so two tasks can claim the same message and both send PDO. That breaks the core dedupe guarantee. We should atomically claimpdo_requested(single-flight/add-if-absent) before registering pending/sending, and add a concurrent regression test to lock this down.🤖 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/pdo.rs` around lines 101 - 132, The current get→get→insert sequence on pdo_requested and pdo_pending_requests is racy: two concurrent callers can both observe absence and both proceed to send PDO; change this to an atomic claim before registering pending and sending by using an add-if-absent/single-flight pattern on self.pdo_requested (e.g., a try_insert or entry-or-insert API) so only the first task wins and others return immediately; after successful atomic claim insert the PendingPdoRequest into self.pdo_pending_requests as before; also add a concurrent regression test that spawns multiple tasks calling the PDO path for the same cache_key and asserts only one sender proceeds (use PendingPdoRequest, pdo_requested, pdo_pending_requests, and the function that currently contains this logic to locate and modify the code).
🤖 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/pdo.rs`:
- Around line 101-132: The current get→get→insert sequence on pdo_requested and
pdo_pending_requests is racy: two concurrent callers can both observe absence
and both proceed to send PDO; change this to an atomic claim before registering
pending and sending by using an add-if-absent/single-flight pattern on
self.pdo_requested (e.g., a try_insert or entry-or-insert API) so only the first
task wins and others return immediately; after successful atomic claim insert
the PendingPdoRequest into self.pdo_pending_requests as before; also add a
concurrent regression test that spawns multiple tasks calling the PDO path for
the same cache_key and asserts only one sender proceeds (use PendingPdoRequest,
pdo_requested, pdo_pending_requests, and the function that currently contains
this logic to locate and modify the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a53bd4e-5d18-45e2-ad84-33f19453859f
📒 Files selected for processing (8)
src/cache_config.rssrc/client.rssrc/client/accessors.rssrc/client/lid_pn.rssrc/client/lifecycle.rssrc/message/receive.rssrc/message/retry.rssrc/pdo.rs
Decrypt-failure tasks are detached per redelivered copy, so the get-then-insert claim let two concurrent copies both pass the gate, and a losing send failure could release the winner's slot. Claim via the single-flight get_with arm already used by dispatch_undecryptable_event: only the claim winner proceeds to send, so only it may release the slot on failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c14c1f7ed5
ℹ️ 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".
| // Nothing moved namespaces, so the retry would hit the exact same | ||
| // state, fail identically, and log a second decrypt failure for | ||
| // every redelivered copy of an undecryptable message. | ||
| if !migrated { |
There was a problem hiding this comment.
When two LID decrypt paths or a mapping learner race, this code drops the LID session lock before migration, so another task can acquire it, move the PN session into the LID slot, and drain the PN side first. This call then gets migrated == false because there is no PN state left, even though the LID state changed after the original failed decrypt, and returns without retrying a decrypt that could now succeed; the message is unnecessarily treated as undecryptable. Consider retrying when the lock was dropped/reacquired, or distinguishing “no PN state existed” from “already migrated by someone else.”
Useful? React with 👍 / 👎.
1 participant
Context
On 2026-06-10 a peer device with cloned Signal state redelivered the same undecryptable pkmsg every ~11s for 3h09 (982 copies of one message id, all
InvalidPreKeyIdbecause its sibling device had already consumed the referenced one-time prekey). The protocol side behaved correctly (retry receipts with fresh keys, sender eventually re-established), but our handling amplified the noise:pdo_pending_requestsonly dedups in-flight requests, so it empties the moment the phone answers (~800ms).Changes
One placeholder resend per message. New
pdo_requestedmemo (24h TTL, 512 entries) gatingsend_pdo_placeholder_resend_request, mirroring the session-lifetimeSetinWAWebNonMessageDataRequestPlaceholderMessageResendUtils(each message id gets at most one resend request per session there). The slot is released when the send itself fails, so a transient error does not block recovery, and it survives a content-less response, since the phone has nothing to share and re-asking cannot help.Skip the migration retry decrypt when nothing migrated.
migrate_signal_sessions_on_lid_discoverynow returns whether anything moved into a LID slot;try_pn_to_lid_migration_decryptskips the second decrypt when it did not. No wire-visible change, the retry receipt still goes out the same.Log levels per the WA Web oracle.
WAWebMsgProcessingDecryptionHandler.canDecryptNextskips the skmsg silently after a retryable pkmsg failure, so the "Skipping skmsg decryption" line drops to debug. The placeholder resend response handler counts a missingwebMessageInfoBytes/message in telemetry with no warning, so "missing message content" drops to info. The per-copy "Max retries reached" line drops to debug since the high-retry warn already fired on the way to the cap.With these, the same storm would produce 1 PDO request, 1 decrypt error per copy and the existing retry receipt cap, instead of 4.6k WARN/ERROR lines and 692 peer messages.
Tests
pdo_request_skipped_when_already_requested: a memoized message does not create a new pending request.pdo_request_failure_releases_once_per_message_slot: a failed send releases the memo so recovery is not permanently blocked.pdo_missing_content_response_clears_pending_but_keeps_memo: a content-less phone response consumes the pending slot but keeps the memo.migration_reports_whether_anything_moved: false with no PN state, true when a PN session moves, false again once drained.cargo fmt,cargo clippy --all-targets -- -D warningsand the full workspace suite (minus e2e) are green.