feat(send): add targeted retransmission controls - #1071
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds cache freshness controls, restructures message and stanza preparation around typed option/request objects, validates extra stanza nodes and retry routes, and introduces public message retransmission APIs with targeted status retry handling. ChangesSend and retransmission pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SendPipeline
participant DeviceRegistry
participant StanzaBuilder
Client->>SendPipeline: send with freshness and stanza options
SendPipeline->>DeviceRegistry: resolve or refresh devices
DeviceRegistry-->>SendPipeline: device snapshot
SendPipeline->>StanzaBuilder: prepare typed DM or group stanza
StanzaBuilder-->>Client: prepared message stanza
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
| Filename | Overview |
|---|---|
| src/retry.rs | Adds typed retransmission routing, resolved sender-key invalidation, and shared session recovery for pairwise retries. |
| src/features/groups.rs | Adds freshness-aware group metadata loading and serialized metadata publication. |
| src/client/device_topology.rs | Serializes device-registry mutations with authoritative refresh publication. |
Reviews (10): Last reviewed commit: "fix(send): validate retry and mapping pu..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 64e448e682
ℹ️ 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: 2
🤖 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/features/groups.rs`:
- Around line 1561-1564: Replace the test phone-like JIDs with reserved
fictitious NANP values matching the 1 + real NPA + 555 + 0100–0199 pattern. In
src/features/groups.rs lines 1561-1564, update the participant JID; in
src/usync.rs lines 400-404, update both the query JID and DeviceListRecord.user
to the same matching compliant value.
In `@src/retry.rs`:
- Around line 3185-3186: Update the fake JIDs in src/retry.rs lines 3185-3186
and 3227 to use NANP fictional numbers with a real NPA, the 555 exchange, and a
0100–0199 line number: replace the direct/requester values with 12025550100 and
12025550100:7, and update the sibling value to 12025550101:9.
🪄 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: 803c9308-ab1e-4ff2-939c-7af12b693264
📒 Files selected for processing (21)
src/cache.rssrc/client/app_state.rssrc/client/device_registry.rssrc/client/messaging.rssrc/client/sender_keys.rssrc/features/groups.rssrc/features/mod.rssrc/features/stanza.rssrc/features/status.rssrc/lib.rssrc/message/special.rssrc/pdo.rssrc/retry.rssrc/send/actions.rssrc/send/mod.rssrc/sender_key_device_cache.rssrc/usync.rswacore/benches/send_receive_benchmark.rswacore/src/send/dm.rswacore/src/send/group.rswacore/src/send/tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/retry.rs`:
- Around line 2597-2695: Replace the invalid fictional NANP numbers in the
listed retry tests with valid numbers using a real NPA, the 555 exchange, and
line numbers 0100–0199. Update all occurrences in
update_local_signal_session_cools_resolved_sender_key_namespace,
status_retransmission_resolution_is_cache_aside_with_pn_fallback,
retransmission_route_validation_is_strict_and_typed, and
resolve_retry_chat_info_broadcast_uses_participant_device, preserving each
existing device suffix.
🪄 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: 447c6313-ab42-42fd-a4a7-3086da82bb5f
📒 Files selected for processing (7)
src/retry.rssrc/send/mod.rswacore/benches/send_receive_benchmark.rswacore/src/iq/usync.rswacore/src/send/encrypt.rswacore/src/send/group.rswacore/src/send/tests.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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/retry.rs (1)
345-350: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject self-targeted status retries before mutating sender-key state.
Status routes satisfy
uses_sender_key(), so Line 345 marks the requester beforeretransmit_status_messagerejects the sending device at Line 864. The API therefore returns an error after changing sender-key tracking, which can trigger unnecessary redistribution later. Move the shared self-target validation beforemark_forget_sender_key; retain the downstream check as defense in depth.Also applies to: 861-866
🤖 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/retry.rs` around lines 345 - 350, In the retry flow around mark_forget_sender_key and retransmit_status_message, perform the shared self-target validation before mutating sender-key state, so self-targeted status retries are rejected without marking the requester. Keep the existing downstream validation near retransmit_status_message as defense in depth.
🤖 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/retry.rs`:
- Around line 345-350: In the retry flow around mark_forget_sender_key and
retransmit_status_message, perform the shared self-target validation before
mutating sender-key state, so self-targeted status retries are rejected without
marking the requester. Keep the existing downstream validation near
retransmit_status_message as defense in depth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d543918-5bcf-4c03-a07c-bf144e5adcba
📒 Files selected for processing (4)
src/features/groups.rssrc/retry.rssrc/usync.rswacore/src/send/tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3769b158a4
ℹ️ 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".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wacore/src/iq/usync.rs`:
- Line 566: Update the require_complete_response field on the public
DeviceListSpec struct to remain public, preserving downstream direct
construction and pattern matching; do not introduce a private field unless the
API is first converted to an explicit constructor or builder flow.
🪄 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: 5734754c-ced5-4f2f-b9f8-d85dfb8ddce3
📒 Files selected for processing (2)
src/usync.rswacore/src/iq/usync.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9afb2bbc5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/usync.rs`:
- Around line 270-277: Make the refresh path that processes users and invokes
clear_device_record atomic by prevalidating every projected device list,
including the empty-list Freshness::Refresh failure, before any identity cleanup
or registry persistence. Only after all users pass validation should the code
perform cleanup and batched writes; preserve the existing error behavior and
avoid leaving prior device records paired with cleared sessions when validation
fails.
🪄 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: 2979c745-071f-41dc-8ba3-a5dd2181e785
📒 Files selected for processing (8)
src/client/device_registry.rssrc/send/mod.rssrc/usync.rswacore/src/adv.rswacore/src/iq/usync.rswacore/src/send/status.rswacore/src/send/tests.rswaproto/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d749f782
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d749f782
ℹ️ 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".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06836f8f3
ℹ️ 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".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5002f893d8
ℹ️ 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".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b8f9a529
ℹ️ 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".
| "recipient is only valid for direct retransmissions".into(), | ||
| )); | ||
| } | ||
| if recipient.is_some_and(|recipient| { |
There was a problem hiding this comment.
When a public direct retransmission supplies chat=A but requester=B without a recipient, or supplies a recipient unrelated to chat, this validation accepts it. The pairwise builder then routes the stanza to the requester/recipient while chat is used only as the recent-message cache key, so the operation can report success after retransmitting A's message in B's conversation. Automatic retry parsing inherently keeps these identities aligned; reject public requests unless the direct chat matches the requester when recipient is absent, or matches the recipient through PN/LID aliases when it is present.
Useful? React with 👍 / 👎.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c5aa3f4e
ℹ️ 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".
| if matches!(route, RetransmissionRoute::Direct) { | ||
| let routing_chat = request.recipient.as_ref().unwrap_or(&request.requester); | ||
| if !self | ||
| .jids_share_user_identity(&request.chat, routing_chat) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier chat-binding report is that chat=A, recipient=A, requester=B still passes this check for any unrelated nonlocal, non-bot user B, because only the chat and recipient identities are compared. The pairwise builder then encrypts A's message to B while labeling A as the recipient, even though with_recipient documents this route for self-device and bot retries. Require a recipient-bearing requester to be a local device or bot before advancing its session.
Useful? React with 👍 / 👎.
| if !self | ||
| .device_topology | ||
| .registry_unchanged_for(topology_generation, |user| { | ||
| device_response_contains_user(response, user) | ||
| }) |
There was a problem hiding this comment.
When a PN→LID mapping changes after the refresh IQ starts, this check still accepts the response because registry_unchanged_for filters out the mapping-only topology entries recorded by LidPnCache. Since learn_device_list_mappings applies the response before this check, an older response can replace the newer PN→LID mapping and publish its device list under the superseded identity, causing subsequent sends to resolve the wrong Signal namespace. Include relevant mapping changes in the CAS/retry decision, while excluding only mappings learned from the response being committed.
Useful? React with 👍 / 👎.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e241c5e248
ℹ️ 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".
| metadata.cache(Arc::clone(&info)).await; | ||
| return Ok(info); |
There was a problem hiding this comment.
When the L1 cache is empty but persisted metadata exists, an add/remove notification arriving while the group IQ is in flight blocks on the cold_metadata lock; this branch then republishes and returns the pre-notification snapshot before the queued handler can apply its change. Fresh evidence beyond the prior warm-response race is that send_group_branch retains this returned Arc without rereading the cache after the notification obtains the lane, and outgoing sends are not otherwise per-chat locked, so the send can compute its phash and SKDM fanout from a removed or missing participant set. Revalidate the snapshot against a notification generation before returning it or retry after releasing the lane.
AGENTS.md reference: AGENTS.md:L27-L29
Useful? React with 👍 / 👎.
| jids = response | ||
| .device_lists | ||
| .into_iter() | ||
| .map(|user| user.user) | ||
| .collect(); |
There was a problem hiding this comment.
When the CAS rejects a response because a relevant PN→LID mapping changed, this replaces the original query identities with the rejected response's canonical JIDs. Fresh evidence beyond the earlier mapping-change report is that the refresh now retries, but seeds that retry from the stale response: if PN X returned LID Y while X concurrently remapped to Z, the next attempt queries obsolete Y and can relearn X→Y or return Y's devices as the authoritative result for X. Keep the original requested JIDs across attempts, or recanonicalize them through the current mapping after the CAS failure.
Useful? React with 👍 / 👎.
1 participant
Summary
Client::retransmit_messageoperation for targeted direct, group, status, and broadcast-list retransmissionsFreshnesspolicy for group metadata and device resolution without clear-then-fetch cache windowsProtocol behavior
The routing and wire policies were checked against the versioned official-client captures for retry jobs, status and broadcast resends, and device/fanout stanza creation.
Performance
Validation
cargo fmt --all -- --checkgit diff --checkcargo check --workspace --all-targets --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --exclude e2e-testsEnvironment-dependent E2E tests were not run locally. Binary-size attribution is left to the repository's dedicated CI comparison.