fix(send): isolate a group device's session-setup failure from the cohort - #996
Conversation
…hort ensure_sessions_for_devices aborted the whole batch (return Err) when any single device's process_prekey_bundle failed. In prepare_group_stanza that Err nulls session_plan, so the entire SKDM fan-out is skipped: every other target — external members AND our own companion devices — gets no SKDM even though its session established fine, while the skmsg still ships and the full cohort is marked has_key=true on ACK. External devices recover via a retry receipt, but an own companion's retry hits mark_forget_sender_key's exclude_own_devices no-op, so it stays keyless and can never decrypt our group messages until an unrelated full rotation. Skip just the failing device (log + continue) instead of aborting, mirroring WA Web's GroupKeyDistributionMsg per-device try/catch: the sessionless device is dropped by the encrypt fan-out and the rest are distributed normally. Residual (separate follow-up): if an own companion's OWN setup fails it is still marked has_key=true from the full-cohort mark; fully closing that needs the own-device mark gated on actual SKDM encryption.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPer-device session setup failures in ChangesSession Setup Failure Isolation
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant EnsureSessions as ensure_sessions_for_devices
participant GoodDevice
participant BadDevice
Caller->>EnsureSessions: request session setup for devices
EnsureSessions->>GoodDevice: spawn session setup task
EnsureSessions->>BadDevice: spawn session setup task
GoodDevice-->>EnsureSessions: session established
BadDevice-->>EnsureSessions: setup failed
EnsureSessions->>EnsureSessions: warn and skip BadDevice
EnsureSessions-->>Caller: return success (GoodDevice sessions ready)
Possibly related PRs
Metadata Look, I need this code to work reliably at scale — billions of messages, zero excuses. This fix is exactly the kind of resilience I want: one bad device shouldn't take down the whole fan-out. That's a win. Ship it, but I expect the tests to be airtight, no exceptions. 🚥 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Confidence score: 3/5
- In
wacore/src/send/encrypt.rs, the setup-failure path can still persist a device ashas_key=trueeven when no SKDM was encrypted, which risks marking broken recipients as ready and causing downstream send/encryption failures; carry setup-failed devices through planning/results (or blockhas_key=truein that branch) before merging. - In
wacore/src/send/tests.rs, duplicated signed PreKeyBundle fixture construction can drift from the canonical setup and let validity-rule changes be updated in one place but not the other; extract a shared helper to keep test expectations aligned and reduce future regression risk.
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="wacore/src/send/encrypt.rs">
<violation number="1" location="wacore/src/send/encrypt.rs:616">
P1: When a device reaches this setup-failure branch, it can still be persisted as `has_key=true` even though no SKDM was encrypted for it. Consider carrying the setup-failed device list through the plan/result or otherwise excluding these failures from `PreparedGroupStanza.skdm_devices`, especially for own companions that do not have a retry-receipt repair path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // GroupKeyDistributionMsg wraps each device's encrypt in try/catch | ||
| // and drops only the failing one; the skipped device just stays | ||
| // sessionless and is skipped by the fan-out below. | ||
| Ok(Err(e)) => { |
There was a problem hiding this comment.
P1: When a device reaches this setup-failure branch, it can still be persisted as has_key=true even though no SKDM was encrypted for it. Consider carrying the setup-failed device list through the plan/result or otherwise excluding these failures from PreparedGroupStanza.skdm_devices, especially for own companions that do not have a retry-receipt repair path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wacore/src/send/encrypt.rs, line 616:
<comment>When a device reaches this setup-failure branch, it can still be persisted as `has_key=true` even though no SKDM was encrypted for it. Consider carrying the setup-failed device list through the plan/result or otherwise excluding these failures from `PreparedGroupStanza.skdm_devices`, especially for own companions that do not have a retry-receipt repair path.</comment>
<file context>
@@ -604,7 +604,18 @@ pub async fn ensure_sessions_for_devices(
+ // GroupKeyDistributionMsg wraps each device's encrypt in try/catch
+ // and drops only the failing one; the skipped device just stays
+ // sessionless and is skipped by the fan-out below.
+ Ok(Err(e)) => {
+ log::warn!("Group session setup failed for a device, skipping it: {e}");
+ }
</file context>
📦 Binary size report
.text per crate
Baseline: |
Deduplicate the verifiable-PreKeyBundle fixture into signed_prekey_bundle() (used by the chain-lock and cohort-isolation tests) and trim the per-device isolation comment to why-only. (review 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: 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/send/tests.rs`:
- Around line 805-828: The new signed_prekey_bundle() helper is not being used
where the same PreKeyBundle setup is still duplicated in established_stores(),
so the refactor is only half applied. Update established_stores() to call
signed_prekey_bundle() instead of manually generating the receiver, spk, opk,
signature, and PreKeyBundle::new data, keeping the helper as the single source
of truth for this shared test setup.
🪄 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: fe4f8edc-0c01-4b5d-869e-950b92958635
📒 Files selected for processing (2)
wacore/src/send/encrypt.rswacore/src/send/tests.rs
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Route established_stores() and setup_session() through signed_prekey_bundle() too, so the verifiable-bundle fixture lives in one place. (review feedback)
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
1 participant
What
Isolate a single group device's session-setup failure so it no longer aborts the SKDM distribution to the whole cohort.
Why (bug)
ensure_sessions_for_devicesreturnedErr(Ok(Err(e)) => return Err(e)) as soon as any device'sprocess_prekey_bundlefailed. Inprepare_group_stanzathatErrnullssession_plan, and the entire SKDM fan-out is gated onif let Some(plan) = session_plan— so every other target gets no SKDM even though its session established fine. The skmsg still ships, the phash covers the full set (server reports no mismatch), and on ACK the full cohort is markedhas_key=true.External members recover via a retry receipt (
mark_forget_sender_key). But an own companion hitsmark_forget_sender_keywithexclude_own_devices=true, which filters own-user JIDs and returns early with no DB write — so the companion stayshas_key=trueforever and can never decrypt our group messages from that device until an unrelated full rotation (participant removal / PN↔LID migration).WA Web's
GroupKeyDistributionMsgwraps each device's encrypt in its own try/catch and drops only the failing one (GroupSkmsgJobswallows theensureE2ESessionserror and continues) — one member's failure never suppresses the rest.How
Ok(Err(e)) => return Err(e)to log and skip that one device. The sessionless device is then skipped by the encrypt fan-out (which already skips devices without a session), and every other device is distributed normally. The encrypt fan-out loop and the 406 batch path were already isolated; this brings the setup path in line.Tests
group_skdm_setup_failure_is_isolated_to_the_bad_device— a group send with one valid-bundle device and one whose bundle fails X3DH:prepare_group_stanzanow succeeds and the good device still receives its pairwise SKDM (participantshas exactly one child). Before the fix, the failing device nulledsession_plan, so there was noparticipantsnode at all (the.expectwould panic).cargo fmt/clippyclean; fullwacoresend suite (96) passes.Residual (separate follow-up)
The primary harm (an unrelated device's failure orphaning own companions) is closed. A narrower window remains: if an own companion's own setup fails, it's still marked
has_key=truefrom the full-cohort mark. Fully closing that needs the own-devicehas_keymark gated on actual SKDM encryption (skdm_encrypted_devices), which I've left as a focused follow-up to keep this send-path change minimal.