feat(voip): add group calls and call links - #1130
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:
📝 WalkthroughWalkthroughThis PR adds end-to-end group VoIP and reusable call-link support, including typed signaling, waiting rooms, authenticated group media, participant controls, reactions, relay reconnection, generation-scoped cleanup, and expanded public APIs. ChangesGroup VoIP platform
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
|
| Filename | Overview |
|---|---|
| src/client/voip.rs | Adds public group and call-link operations with serialized, generation-scoped admission registration and bounded pre-ACK buffering. |
| src/handlers/call.rs | Routes group signaling through creator, roster, and current-generation authorization before mutating call state. |
| wacore/src/voip/registry.rs | Extends call lifecycle bookkeeping with group rosters, authorization, generations, bounded controls, and media-state publication. |
| wacore/src/voip/group_media.rs | Implements authenticated participant-aware group media routing and epoch-scoped key handling. |
| wacore/src/stanza/group_call.rs | Adds typed parsers and builders for group calls, call links, waiting rooms, and participant controls. |
Sequence Diagram
sequenceDiagram
participant App
participant Client
participant Service as Call Service
participant Handler as Call Handler
participant Registry as Call Registry
participant Media as Group Media
App->>Client: Join call link
Client->>Service: Join request
Service-->>Client: ACK with call ID and admission state
Handler->>Client: Buffer controls racing registration
Client->>Registry: Register generation
Client->>Registry: Replay validated buffered state
Registry->>Media: Publish roster, relay, and epoch
Media-->>App: Participant-aware media and events
Reviews (47): Last reviewed commit: "fix(voip): close group admission races" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
🚨 Per-PR size budget exceeded (Δ stripped ≤ 64.00 KiB, Δ .text ≤ 32.00 KiB):
The Baseline: |
jlucaso1
added
the
size-increase-ok
![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: 2d5e867381
ℹ️ 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".
2d5e867 to
67f37b6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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: 67f37b61c1
ℹ️ 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: c876eb6117
ℹ️ 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: 35
🤖 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/client/voip.rs`:
- Around line 733-760: The execute_call_service_request function should define
and use a named CALL_SERVICE_REQUEST_TIMEOUT constant with a 10-second Duration
instead of inlining the value in the timeout call. Also simplify the send_node
error propagation by replacing the explicit if-let return block with the ?
operator.
- Around line 642-665: Update the heartbeat loop in the runtime task around
build_waiting_room_heartbeat and send_node so a single send failure is treated
as transient: log the failure with warn! and continue retrying while the call
remains in WaitingRoom. Only terminate the loop after the defined terminal
failure condition, and surface that terminal failure through the existing
CallEvent mechanism used by other failure paths in this file.
- Around line 380-384: Validate that join.call_id is non-empty in the link_join
flow before constructing or registering the CallSession; reject the join with
the existing fatal/error handling used for invalid call IDs elsewhere, ensuring
registry.insert never receives a session keyed by an empty ID.
- Around line 396-441: Remove the hand-written registry.remove_if_current calls
from the call-link join error branches after CallLinkRegistrationGuard::new
creates registration. Let registration’s drop behavior handle cleanup for
missing waiting-room state, rejected identity, heartbeat failure,
fanout_group_epoch failure, and failed send_group_epoch retention; simplify the
rekey_requested branch to propagate the epoch error directly while preserving
all existing error results.
- Around line 712-731: Update the call-link parsing logic around the URL prefix
and path splitting to remove any query string or fragment from the pasted link
before validating and extracting the token, while preserving rejection of
malformed URLs and empty tokens. In the same flow, replace the unreachable
token.unwrap_or_default() fallback with the already-validated token value.
In `@src/handlers/call.rs`:
- Around line 311-336: The rekey failure branch in the update handling flow must
surface the failure instead of only logging it. Update the Err arm around
fanout_group_epoch to emit the appropriate CallEvent or schedule the established
retry mechanism, ensuring the update is not treated as successfully applied
while peers lack a fresh epoch.
In `@src/voip/facade.rs`:
- Around line 196-220: Compute the group-call boolean once from the resolved
group value in the incoming-answer flow, and reuse it for session setup, peer
JID selection, invite-device seeding, engine construction, and
build_answer_signaling instead of checking different sources. Update
build_answer_signaling to accept this boolean parameter and remove its
incoming.group-based derivation. Also remove the duplicate video.is_some()
variable by retaining one shared video flag.
- Around line 625-657: Consolidate the duplicated audio, encoded_audio, and
video builder methods shared by AcceptCall, OutgoingGroupCall, GroupBoundCall,
and CallLinkCall into one reusable macro or shared endpoint helper, while
preserving each builder’s public API and behavior. Apply the shared
implementation to all four builders and remove their byte-identical method
copies.
- Around line 686-724: Update the target-resolution flow in the group-call setup
loop to resolve all non-LID targets concurrently, using the existing client
resolver and preserving input order where practical. Then process the resolved
results sequentially for local-user checks, deduplication, participant limits,
and existing error behavior; avoid awaiting resolve_recipient_to_lid inside the
sequential validation loop.
- Around line 782-810: Construct GroupOfferTeardown immediately after the
successful self.client.send_node(offer) call and before awaiting the response
timeout, using the existing call_id and own_lid values. Remove the later
construction after the ack is received, while preserving the existing disarm()
success path and all response error handling.
- Around line 2306-2310: Replace the unbounded group control channel created in
the call setup flow with a bounded channel using a new
GROUP_CTL_CHANNEL_CAPACITY constant declared alongside the other channel
capacity constants. Preserve the existing group_tx registration through
call_registry().set_group_control_sender and the group_rx assignment, relying on
the registry’s existing try_send behavior for full queues.
- Around line 1325-1355: Ensure every copy of the call key is zeroized,
including the protobuf-owned clone created in the Message passed to
MessageUtils::encode_and_pad. Make raw_epoch cancellation-safe by wrapping it in
a zeroize-on-drop type or equivalent Drop guard, then remove the scattered
manual wipes while preserving zeroization of padded and all normal error/return
paths.
In `@src/voip/mod.rs`:
- Around line 41-44: Extend the public re-export block in src/voip/mod.rs to
include GroupCallRelay, GroupCallRelayEndpoint, GroupCallEncRekey, and
GROUP_CALL_MAX_PARTICIPANTS from wacore::types::group_call, alongside the
existing group-call symbols. Preserve the current facade structure so all public
types used by GroupCallUpdate and CallAction remain nameable through this
module.
In `@wacore/src/stanza/call.rs`:
- Around line 82-86: Update the media-offer parsing flow around
parse_media_offer to pass the routed sender participant when one is available,
rather than always using from. Preserve from as the fallback when participant is
absent, so GroupCallDevice::new receives the actual peer JID and existing
non-participant flows remain unchanged.
In `@wacore/src/stanza/group_call.rs`:
- Around line 776-812: Apply validate_call_identity consistently to the
waiting-room builders build_waiting_room_toggle, build_waiting_room_heartbeat,
and build_waiting_room_admit, as well as build_raise_hand and
build_screen_share. Align these builders with the validated Result-returning
contract used by build_initial_group_offer, build_group_invite_offer, and
build_group_enc_rekey, and propagate validation failures instead of emitting
nodes with empty call IDs, request IDs, or creators.
- Around line 1509-1557: Add tests in the existing group-call test module for
build_call_link_query and parse_call_link_query_ack, including an
acknowledgement without a waiting_room element to verify parsing does not
require that tag. Extend parse_indexed_tokens coverage with out-of-order token
IDs and assert all valid entries are preserved rather than truncated.
- Around line 463-473: Update the token collection logic around the index
calculation and values.resize_with so out-of-order token IDs never truncate
previously parsed entries. Only grow values when index + 1 exceeds its current
length, then assign value at index while preserving existing entries.
- Around line 77-97: Update build_active_group_accept and its callers,
especially accept_group_invite(), to carry IncomingCall::is_video through the
acceptance flow and populate the accept video field consistently with
build_active_group_preaccept. If audio-only acceptance is intentional, document
that rationale at this decision point instead.
- Around line 676-700: Restructure the branching around the waiting and group
values so the group branch is only constructed when a valid group is present,
eliminating the expect call and the separate waiting.is_none() &&
group.is_none() guard. Preserve the existing field extraction and error behavior
for acknowledgements missing both variants, while ensuring server input cannot
trigger a panic.
- Around line 201-219: Update the video capability selection in
build_group_users so creator detection compares the creator and device user
identities via their non-AD JIDs, rather than requiring an exact device JID
match. Preserve the existing CAPABILITY_VIDEO_OFFER and
CAPABILITY_STANDARD_OPUS_VIDEO_OFFER mappings for any device owned by the
creator, including when call_creator is an AD JID with device 0.
In `@wacore/src/types/call.rs`:
- Around line 227-232: Box the GroupUpdate payload by changing its update field
to Box<GroupCallUpdate>, preserving the existing GroupCallUpdate type and
variant semantics. Update callers in the call-stanza handling around the
GroupUpdate construction to convert the value with .into(), and adjust any other
constructors or pattern matches that require the boxed field.
In `@wacore/src/voip/app_data.rs`:
- Around line 201-214: Expand
decoder_skips_unknown_fields_and_rejects_invalid_inputs with assertions covering
TooLarge for oversized reaction encoding and length-delimited decoding,
MAX_APP_DATA_BYTES rejection, InvalidUtf8 for malformed UTF-8 reaction data, and
Malformed for a 10-byte varint overflow. Construct each input through the
existing encoding/decoding APIs and preserve the current unknown-field and
invalid-input checks.
In `@wacore/src/voip/driver.rs`:
- Around line 37-70: Replace the manual raw_epoch.fill(0) call in
GroupRawEpoch’s Drop implementation with the existing zeroize dependency,
invoking Zeroize on the buffer so its contents are reliably erased before
release.
- Around line 732-753: Update the group-control handling in the drive loop so
recoverable errors from apply_group_update, apply_group_raw_epoch, and
send_group_reaction are treated as no-ops rather than breaking the 'drive loop.
Preserve the existing fatal behavior only for errors that represent
unrecoverable call-state failures, matching the policy used by the nearby
video_ctl arm.
In `@wacore/src/voip/engine.rs`:
- Around line 2703-2725: Add a negative-path test near group_engine, such as
group_media_is_gated_until_an_authenticated_epoch_installs, that configures and
starts a video group without applying an epoch, drains startup events, and
verifies both video and audio input produce zero transmits under the zero
bootstrap key. Then install the authenticated epoch with apply_group_raw_epoch
and verify video transmission resumes, pinning the group_epoch_ready gate
behavior.
- Around line 933-940: Update the configure_group call in the direct-call
promotion path to propagate its original error instead of mapping every failure
to GroupMediaError::Pipeline. Preserve the existing success return and rely on
GroupMediaError’s existing EngineError conversion so BadCallKey, InvalidEpoch,
and inner GroupMedia causes remain available.
- Around line 866-874: In the MediaPipeline construction within the app-data
setup, replace the samples_per_packet value derived from APP_DATA_RETRANSMIT_MS
with a dedicated constant for the app-data RTP timestamp stride. Define or reuse
that independent constant near the related configuration, while leaving
APP_DATA_RETRANSMIT_MS exclusively responsible for retransmit cadence.
In `@wacore/src/voip/group_media.rs`:
- Around line 675-896: Add tests covering the two missing risks: drive
GroupMediaRegistry epoch buffering beyond MAX_BUFFERED_EPOCHS and assert the
oldest buffered epoch is evicted, and extend the video coverage around
peer_video_sender and unprotect_video to use multiple participants whose derived
SSRCs could collide, asserting packets are routed to the correct participant.
Keep the tests focused on eviction and per-participant SSRC routing.
- Around line 496-540: Update the VideoPipeline construction in the video branch
to pass the previously computed participant-derived video_ssrc instead of
deriving an SSRC from self_lid. Keep the existing video conditional and pipeline
parameters unchanged so its key matches the receiver’s video route.
- Around line 239-255: Update the audio-to-video receiver rebuild in the
participant reuse logic to preserve the existing app-data pipeline alongside
existing.audio. Move existing.app_data into rebuilt.app_data after
build_receiver returns, so both depacketization state and ROC survive the
rebuild.
- Around line 311-326: Update the MAX_BUFFERED_EPOCHS eviction in the
pending_epochs buffering logic to remove the entry with the greatest transaction
ID, not the lowest. Preserve insertion of the new future epoch and the existing
InvalidEpoch handling, so the nearest pending epoch remains available for
install_best_pending_epoch.
In `@wacore/src/voip/group.rs`:
- Around line 213-304: Add focused tests for apply_waiting_room covering call
identity mismatch, the stale result when both existing and incoming snapshots
have transaction IDs and the incoming one is older, and the rule that a
transaction-less update always applies. Include a sequence that applies an
initial room, rejects an older transaction as Stale, then accepts a
transaction-less update.
In `@wacore/src/voip/registry.rs`:
- Around line 257-314: Extract the repeated group-state initialization into a
private CallEntry::group_mut helper that lazily creates GroupCallState from the
entry’s session identity. Update apply_waiting_room, set_raised_hand,
set_screen_share, and apply_group_update to use entry.group_mut() instead of
duplicating the cloning and get_or_insert_with logic, preserving their existing
return behavior.
- Around line 528-538: Update set_group_invite_peer_device to accept a
generation parameter and only set entry.group_invite_peer_device when
entry.generation matches it, consistent with sibling setters. Thread
handle.generation through the corresponding caller in the voip facade so stale
call handles cannot modify replacement entries.
In `@wacore/tests/group_call_builders_test.rs`:
- Around line 11-13: Extend the tests around the GroupCallDevice builder and the
ScreenShare builder path at line 83 to assert every omitted field’s expected
default after build, including Option fields and wire-level defaults. Keep the
existing explicitly provided values asserted, and verify both builder paths
produce the correct payload defaults.
🪄 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: 3714eb46-c1b9-495d-bff8-9a899214a6cd
📒 Files selected for processing (29)
src/client/voip.rssrc/handlers/call.rssrc/request.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rswacore/src/stanza/call.rswacore/src/stanza/group_call.rswacore/src/stanza/mod.rswacore/src/types/call.rswacore/src/types/group_call.rswacore/src/types/mod.rswacore/src/voip/app_data.rswacore/src/voip/audio.rswacore/src/voip/demux.rswacore/src/voip/driver.rswacore/src/voip/e2e_srtp.rswacore/src/voip/engine.rswacore/src/voip/group.rswacore/src/voip/group_audio.rswacore/src/voip/group_media.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rswacore/src/voip/stun.rswacore/tests/group_call_builders_test.rs
c876eb6 to
0a9440c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a9440c46e
ℹ️ 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: 15
♻️ Duplicate comments (1)
wacore/src/voip/driver.rs (1)
777-788: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winThe epoch arm's fatal/recoverable polarity is backwards, and it hangs up on a race the protocol expects.
apply_group_raw_epochreturnsGroupMediaError::Pipelinewhenself.groupis stillNone(engine.rs Line 1013) — i.e. a decrypted epoch that arrives before the first PID-bearing roster. That is a routine ordering race;GroupMediaRegistryships an entire boundedpending_epochsbuffer built to absorb it. Except that buffer only becomes reachable once the group is configured, so today the race takes thebreak 'driveand drops the user out of the call.Meanwhile
InvalidEpochandConflictingEpoch— actual bad or hostile server data — are treated as merely "rejected". That's exactly inverted from theUpdatearm right above, which is the correct shape.No epoch is worth a dropped call. The next roster's
install_best_pending_epochrecovers, and if it doesn't, the media gate keeps the zero bootstrap key off the wire anyway.🐛 Treat epoch failures as rejections, matching the reaction arm
Some(GroupControl::RawEpoch(epoch)) => { - if let Err(error) = eng + // An epoch racing ahead of the first roster, or one the registry refuses, + // is recoverable: the next roster installs from the pending buffer, and + // outbound media stays gated until an epoch commits. + if eng .apply_group_raw_epoch(epoch.transaction_id, epoch.as_bytes()) + .is_err() { - if matches!(error, crate::voip::GroupMediaError::Pipeline) { - break 'drive; - } let _ = channels.events.try_send(CallEvent::GroupControlRejected { control: engine::GroupControlKind::Epoch, }); } }Add a test that pushes
GroupControl::RawEpochinto a driver whose engine has no group configured yet and asserts the loop keeps servicing relay traffic.🤖 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 `@wacore/src/voip/driver.rs` around lines 777 - 788, Update the RawEpoch handling in the driver loop around apply_group_raw_epoch to treat every GroupMediaError, including Pipeline, as a rejected control: send GroupControlRejected and continue servicing relay traffic instead of breaking 'drive. Match the existing Update arm’s error polarity, and add a test that submits RawEpoch before the engine has a configured group and verifies the driver loop remains active.
🤖 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 `@Cargo.toml`:
- Line 125: Update the zeroize dependency declaration to explicitly enable its
alloc feature while retaining default-features = false, ensuring
Zeroizing<Vec<u8>> in the voip facade is supported for the workspace and
published crate configuration.
In `@src/client/voip.rs`:
- Around line 461-481: Update set_approval_required to capture the call’s
generation before execute_call_service_request, then apply the result through a
new set_waiting_room_enabled_if_current registry method. Add that method beside
set_waiting_room_enabled in the call registry, returning whether the expected
generation was still current; if it returns false, return CallError::Media("call
was replaced while applying group control") instead of Ok(()), matching
set_hand_raised and set_screen_share.
- Around line 672-674: Update the screen-share transition handling around the
state check and registry.send_video_ctl call to send
VideoControl::RequireKeyframe for both starting and stopping transitions, not
only ScreenShareState::Started. Update the related test to expect a keyframe
request when stopping screen share, unless the existing no-request behavior is
intentionally retained; in that case, document the decision at the state check.
In `@src/handlers/call.rs`:
- Around line 203-227: Update the group invite handling block around
set_group_invite_peer_device to use the file’s routed_call_sender resolver
instead of call.from when constructing GroupCallDevice, preserving the
resolver’s fallback to from for 1:1 calls and correctly resolving the
participant device for call-scoped group stanzas.
- Around line 708-741: Update decrypt_group_epoch to wrap the decrypted
plaintext and extracted raw_epoch key material in the existing Zeroizing type,
matching fanout_group_epoch. Remove manual zeroize calls and preserve the
current validation and return behavior so cancellation, decode failures, short
keys, and normal drops wipe the bytes through destructors.
In `@src/voip/facade.rs`:
- Line 3457: Update the test PN initialized in the Jid::new call to use the
repository’s fictional NANP format: country code 1, a real non-555 NPA, exchange
555, and a line number from 0100 through 0199. Keep the existing Server::Pn
value unchanged.
In `@wacore/src/stanza/call.rs`:
- Around line 248-266: Update the five group-action arms in the call-action
match—GroupUpdate, EncRekey, WaitingRoomUpdate, RaiseHand, and ScreenShare—to
enforce the same attrs.finish() validation as the other arms. If these arms
intentionally allow unknown attributes because their sub-parsers validate them,
document that contract directly at this match decision point instead.
- Around line 169-188: The capability version parsing in the peer-device
construction must distinguish an absent ver from an invalid one. Update the
attrs.optional_u64("ver") handling in the peer_device closure so a missing
attribute retains the wire default of 1, while parse failures or values that
cannot convert to u32 produce None; do not use unwrap_or(1) after the
conversion.
In `@wacore/src/stanza/group_call.rs`:
- Around line 148-158: Add an inline comment at the build_group_users call in
the invite path explaining that the roster intentionally uses the
server-authoritative snapshot with None and false, even for video invites,
rather than upgrading other participants’ capability blobs; keep the existing
video_offer_node behavior unchanged.
- Around line 970-987: Update parse_screen_share to convert the server-provided
state_value with i32::try_from before calling ScreenShareState::try_from,
removing the lossy as i32 cast. Propagate a conversion failure and preserve the
existing unsupported-state error reporting for values that fit in i32.
In `@wacore/src/voip/engine.rs`:
- Around line 1063-1076: Re-arm the allocation safety net when
refresh_group_allocate detects a relay address change: thread the current
timestamp through apply_group_update/configure_group into
refresh_group_allocate, or use an equivalent pending-reallocation mechanism, and
set allocate_deadline alongside allocated = false. Ensure on_timeout and on_stun
can process timeout and allocation-error responses for the replacement relay,
and add a test that migrates the relay, advances beyond ALLOCATE_TIMEOUT_MS
without an acknowledgement, and asserts RelayAllocateTimedOut.
In `@wacore/src/voip/group_audio.rs`:
- Around line 128-140: Update GroupAudio’s push method to make invalid chunk
lengths immediately diagnosable instead of returning the same None used for
insufficient samples. Add an appropriate debug assertion or otherwise
distinguish the wrong-sized input while preserving the existing buffering
behavior for valid GROUP_MIX_CHUNK_SAMPLES chunks.
In `@wacore/src/voip/group_media.rs`:
- Around line 515-523: Replace the hardcoded samples_per_packet value in the
MediaPipeline construction with the shared APP_DATA_RTP_TIMESTAMP_STRIDE
constant, promoting it to pub(crate) in engine.rs or moving it to an already
shared module so both engine.rs and group_media.rs use the same definition.
In `@wacore/src/voip/registry.rs`:
- Around line 724-757: Add generation-scoped variants alongside
send_group_update and send_group_epoch, accepting the expected generation and
rejecting entries whose generation does not match; keep the existing
call-id-only methods unchanged for incoming signaling. Update the local-commit
callers in client voip flow and GroupRekeyTeardown’s fanout_group_epoch to use
the new variants, and factor send_group_epoch’s delivery/buffering logic into a
private helper shared by both entry points.
In `@wacore/src/voip/ssrc.rs`:
- Around line 25-36: Pin the positional contract between
derive_wasm_relay_stream_ssrcs and stun::WASM_STREAM_SLOTS by adding the
requested #[cfg(test)] crate-visible wasm_stream_slot_words accessor in stun.rs,
then add a test near WASM_RELAY_STREAM_SLOT_WORDS that asserts it equals the
relay slot-word array. Keep the production ordering unchanged.
---
Duplicate comments:
In `@wacore/src/voip/driver.rs`:
- Around line 777-788: Update the RawEpoch handling in the driver loop around
apply_group_raw_epoch to treat every GroupMediaError, including Pipeline, as a
rejected control: send GroupControlRejected and continue servicing relay traffic
instead of breaking 'drive. Match the existing Update arm’s error polarity, and
add a test that submits RawEpoch before the engine has a configured group and
verifies the driver loop remains active.
🪄 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: 13e84247-488c-4f2e-a0f9-6da826b0514c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlsrc/client/voip.rssrc/handlers/call.rssrc/request.rssrc/voip/facade.rssrc/voip/mod.rssrc/voip/transport.rswacore/Cargo.tomlwacore/src/stanza/call.rswacore/src/stanza/group_call.rswacore/src/stanza/mod.rswacore/src/types/call.rswacore/src/types/group_call.rswacore/src/types/mod.rswacore/src/voip/app_data.rswacore/src/voip/audio.rswacore/src/voip/demux.rswacore/src/voip/driver.rswacore/src/voip/e2e_srtp.rswacore/src/voip/engine.rswacore/src/voip/group.rswacore/src/voip/group_audio.rswacore/src/voip/group_media.rswacore/src/voip/h264.rswacore/src/voip/mod.rswacore/src/voip/registry.rswacore/src/voip/rtp.rswacore/src/voip/session.rswacore/src/voip/ssrc.rswacore/src/voip/stun.rswacore/src/voip/transport.rswacore/tests/group_call_builders_test.rs
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 (4)
wacore/src/voip/engine.rs (1)
1677-1684: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBind allocation acknowledgements to the current request.
on_stunaccepts any allocate/binding success whileallocatedis false; it does not validate the STUN transaction ID or relay generation. After migration, a delayed response from the old relay can mark the replacement relay as allocated and disable the timeout even though the replacement never acknowledged our request.Track the active allocation transaction/generation and ignore stale responses.
🤖 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 `@wacore/src/voip/engine.rs` around lines 1677 - 1684, Update the allocation-success handling in on_stun to validate the response against the active allocation transaction ID and relay generation before setting allocated, clearing allocate_deadline, logging, or emitting RelayAllocated. Track and refresh these identifiers whenever a new allocation request or relay migration starts, and ignore acknowledgements that do not match the current request.wacore/src/voip/registry.rs (2)
287-295: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
set_waiting_room_enabledgeneration-scoped like the other local waits.This commit mutates the current waiting-room group by
call_id, so a late local acknowledgement from a superseded same-call-id call can flip the replacement call’swaiting_room_enabledstate. Addgenerationand filter it likeset_raised_hand_if_current.🤖 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 `@wacore/src/voip/registry.rs` around lines 287 - 295, Update set_waiting_room_enabled to accept a generation parameter and require the registry entry’s generation to match before mutating the waiting-room group. Mirror the generation filter used by set_raised_hand_if_current, while preserving the existing enabled update and false result when no matching current entry exists.
698-726: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not discard pending group roster or epoch state on queue pressure.
set_group_control_sender()takes the current roster snapshot andpending_group_epochbefore usingtry_send. With the bounded group-control channel full, both messages can be dropped and the engine is left without the authoritative roster/epoch state. Keep queue-affected state until delivery succeeds, matching the authoritative state semantics ofsend_group_update()/send_group_epoch().🤖 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 `@wacore/src/voip/registry.rs` around lines 698 - 726, Update set_group_control_sender so the roster snapshot and pending_group_epoch are not removed from the registry before try_send succeeds. Retain each queue-affected state when the bounded channel is full, and clear it only after the corresponding GroupControl message is delivered, matching send_group_update() and send_group_epoch() authoritative-state behavior.src/handlers/call.rs (1)
307-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFive spots now do "check authorization, warn, and reject" — two different ways.
GroupUpdate(309-321) andWaitingRoomUpdate(427-439) reject unauthorized senders via a match guard;EncRekey(392-426),RaiseHand(462-492), andScreenShare(493-523) do the identical check-warn-dispatch_call = falsedance inline in the arm body. Same security-critical shape, five call sites, two syntaxes. A tiny helper — e.g.fn authorize_or_warn(registry, call_id, call_creator, sender, action_kind) -> bool— would collapse this into one reviewable place, which matters more for an authorization gate than for ordinary business logic: one bug fixed once instead of potentially missed in four other copies.🤖 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/handlers/call.rs` around lines 307 - 525, Centralize the repeated authorization, warning, and rejection logic in a small helper near the call-dispatch handling, such as authorize_or_warn, accepting the registry, call identifiers, creator, sender, and action label. Replace the guarded GroupUpdate and WaitingRoomUpdate arms plus inline checks in EncRekey, RaiseHand, and ScreenShare with this helper, preserving each action’s existing warning context and dispatch_call behavior.
♻️ Duplicate comments (1)
src/handlers/call.rs (1)
203-227: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill using
call.frominstead ofrouted_call_sender— this bug survives right next to the code that just proved the fix.This exact spot was flagged before: for a group-scoped
<capability>stanza,call.fromis the@callwrapper JID, not a device —set_group_invite_peer_deviceends up recording a JID that can never match a real participant device. The new authorization guards added a few lines down (309-321, 427-439) lean onrouted_call_sender(&call)for precisely this reason. Look, I don't love double standards in my own codebase — ifrouted_call_senderis good enough for authorizing group snapshots, it's good enough here too. Ship the fix.♻️ Route through the same sender resolver
client.call_registry().set_group_invite_peer_device( call.action.call_id(), generation, - GroupCallDevice::new(call.from.clone()) + GroupCallDevice::new(routed_call_sender(&call)) .with_capability(version, bytes.to_vec()), );🤖 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/handlers/call.rs` around lines 203 - 227, Update the group capability handling block to pass the resolved device sender from routed_call_sender(&call) into GroupCallDevice::new instead of call.from. Keep the existing generation, capability parsing, and set_group_invite_peer_device flow unchanged.
🤖 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/handlers/call.rs`:
- Around line 307-525: Centralize the repeated authorization, warning, and
rejection logic in a small helper near the call-dispatch handling, such as
authorize_or_warn, accepting the registry, call identifiers, creator, sender,
and action label. Replace the guarded GroupUpdate and WaitingRoomUpdate arms
plus inline checks in EncRekey, RaiseHand, and ScreenShare with this helper,
preserving each action’s existing warning context and dispatch_call behavior.
In `@wacore/src/voip/engine.rs`:
- Around line 1677-1684: Update the allocation-success handling in on_stun to
validate the response against the active allocation transaction ID and relay
generation before setting allocated, clearing allocate_deadline, logging, or
emitting RelayAllocated. Track and refresh these identifiers whenever a new
allocation request or relay migration starts, and ignore acknowledgements that
do not match the current request.
In `@wacore/src/voip/registry.rs`:
- Around line 287-295: Update set_waiting_room_enabled to accept a generation
parameter and require the registry entry’s generation to match before mutating
the waiting-room group. Mirror the generation filter used by
set_raised_hand_if_current, while preserving the existing enabled update and
false result when no matching current entry exists.
- Around line 698-726: Update set_group_control_sender so the roster snapshot
and pending_group_epoch are not removed from the registry before try_send
succeeds. Retain each queue-affected state when the bounded channel is full, and
clear it only after the corresponding GroupControl message is delivered,
matching send_group_update() and send_group_epoch() authoritative-state
behavior.
---
Duplicate comments:
In `@src/handlers/call.rs`:
- Around line 203-227: Update the group capability handling block to pass the
resolved device sender from routed_call_sender(&call) into GroupCallDevice::new
instead of call.from. Keep the existing generation, capability parsing, and
set_group_invite_peer_device flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b59ac03a-b90f-44e2-94ed-04f8aeab9462
📒 Files selected for processing (3)
src/handlers/call.rswacore/src/voip/engine.rswacore/src/voip/registry.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24fc56459
ℹ️ 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: e17509309b
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
wacore/src/voip/group_media.rs (1)
314-329: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThis eviction can throw away the epoch you JUST buffered. Fix it before it ships.
Walk through it: insert the new epoch first, THEN look for the "farthest" (largest) key to evict — but the map you're evicting from now includes the entry you just added. Rekey transaction ids increase monotonically in practice, so the newest arrival is almost always the new maximum. That means the epoch you just buffered gets immediately evicted, and the buffer quietly keeps whatever old, more speculative epochs it already had. Once the buffer saturates, every subsequent epoch is a no-op insert-then-evict — we silently stop accepting new group rekeys until the roster catches up and prunes the backlog. That's the opposite of "keep the imminent one" that we fixed this for last time.
Check capacity before you insert, not after.
🔧 Evict before inserting, and skip buffering an entry that would be evicted anyway
if self .roster_transaction .is_none_or(|roster| transaction_id > roster) { - self.pending_epochs.insert( - transaction_id, - EpochKey::new(raw_epoch).ok_or(GroupMediaError::InvalidEpoch)?, - ); - if self.pending_epochs.len() > MAX_BUFFERED_EPOCHS { - let Some(farthest) = self.pending_epochs.keys().next_back().copied() else { - return Err(GroupMediaError::InvalidEpoch); - }; - self.pending_epochs.remove(&farthest); - } + if self.pending_epochs.len() >= MAX_BUFFERED_EPOCHS + && let Some(&farthest) = self.pending_epochs.keys().next_back() + && farthest > transaction_id + { + // Only evict an existing entry that is more speculative than the one arriving. + self.pending_epochs.remove(&farthest); + } + if self.pending_epochs.len() < MAX_BUFFERED_EPOCHS { + self.pending_epochs.insert( + transaction_id, + EpochKey::new(raw_epoch).ok_or(GroupMediaError::InvalidEpoch)?, + ); + } return Ok(GroupEpochApply::Buffered); }🤖 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 `@wacore/src/voip/group_media.rs` around lines 314 - 329, Update the pending_epochs buffering logic in the roster_transaction branch to enforce capacity before inserting the new transaction_id. Evict the existing farthest key when the buffer is full, and if the incoming transaction_id would be that farthest entry, skip buffering it and return the existing buffered outcome. Preserve EpochKey validation and normal insertion behavior when capacity remains.src/voip/facade.rs (1)
916-947: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winWe can't ship a call-link join that can silently orphan a live call on the server. That's not acceptable.
Look, admission "commits this endpoint server-side" — your own comment says so. But
teardown(the thing that actually sends<terminate>) isn't created until AFTER the admission-wait loop (line 947). Everything betweenjoin_call_link_with_audioreturning (which already disarmed its own guard) and line 947 is unprotected: thecleanupscopeguard at 925-930 only rips the LOCAL registry entry, it never tells the server we're bailing. A user backing out of a waiting room, or the whole future getting cancelled while parked onlistener.await, leaves the server thinking the call is still live. We fixed this exact class of bug forOutgoingGroupCall's offer-ack window already — this is the same mistake in a different flow.Move the terminate-armed guard up before the admission loop, not after it.
🔧 Arm the terminate before the admission wait, not after
let cleanup = scopeguard::guard( (registry.clone(), join.call_id.clone(), generation), |(registry, call_id, generation)| { registry.remove_if_current(&call_id, generation); }, ); + // Admission has committed this endpoint server-side. Keep a call-scoped terminate armed + // across the admission wait itself, or a cancelled/failed wait leaves the call live on the + // server with nothing to terminate it. + let mut teardown = GroupOfferTeardown::new(self.client, &join.call_id, &join.call_creator); let update = loop { let listener = registry .listen_group_update(&join.call_id, generation) .ok_or(CallError::Media("call-link ended before admission"))?; if let Some(update) = registry .group_state_if_current(&join.call_id, generation) .and_then(|state| state.snapshot().cloned()) .filter(|update| update.relay.is_some()) { break update; } listener.await; }; - // Admission has committed this endpoint server-side. Keep a call-scoped terminate armed - // until the relay driver owns the join, covering setup errors and cancellation. - let mut teardown = GroupOfferTeardown::new(self.client, &join.call_id, &join.call_creator); if update.call_creator != join.call_creator {🤖 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/voip/facade.rs` around lines 916 - 947, Move the GroupOfferTeardown guard construction in the call-link join flow to immediately after join_call_link_with_audio succeeds and before the admission-wait loop. Keep it armed while awaiting listener updates so cancellation, setup errors, or admission failure send server-side termination; retain the existing local registry cleanup separately.wacore/src/voip/driver.rs (2)
630-637: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClearing
awaiting_video_keyframeon reconnect is backwards.We just threw away every queued video batch — possibly mid-access-unit — and then tell the pipeline "no keyframe needed." Every other purge path here (
purge_unstarted_video,purge_queued_video,enqueue_batchoverflow) sets this flag totruefor exactly this reason. After a relay migration the next AU admitted can be a delta whose references never made it onto the old relay, so the peer decodes garbage until the next natural IDR. Set it, don't clear it.🐛 Proposed fix
Output::ReconnectRelay(endpoint) => { // Anything queued before this intent targets the retired relay. Later outputs in // the same drain include the fresh Allocate and are retained for the new channel. send_queue.clear(); pending_video.clear(); - awaiting_video_keyframe = false; + // Discarded batches may have been mid-AU on the retired relay, so the + // replacement path must not resume on a dependent frame. + awaiting_video_keyframe = true; reconnect_to = Some(endpoint); }The engine side needs the matching move:
commit_group_allocatedoesn't callrequire_video_keyframe()on an endpoint change either, so the outbound gate stays open there too.🤖 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 `@wacore/src/voip/driver.rs` around lines 630 - 637, In the `Output::ReconnectRelay` handling, change the `awaiting_video_keyframe` assignment to require a keyframe after clearing queued video during relay migration. Also update `commit_group_allocate` to call `require_video_keyframe()` when the endpoint changes, keeping the outbound gate closed until a fresh keyframe is available.
662-672: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
transport.reconnect(endpoint).awaitis an unbounded blocking call on the drive loop.This sits outside the
select_biased!, so while the platform is dialing the replacement relay nothing else runs — no keepalive, no playout tick, no inbound drain. A dial that hangs (DNS, a firewalled endpoint, a wedged ICE agent) freezes the whole call with no timeout and no terminal event, which is the exact failure mode the send path was decoupled to avoid. Bound it withrt.sleep-based race or push the reconnect into the select as a fused future.🤖 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 `@wacore/src/voip/driver.rs` around lines 662 - 672, The reconnect await in the drive loop must be time-bounded so a hung dial cannot pause keepalive, playout, inbound draining, or terminal handling. Update the reconnect handling around transport.reconnect and the drive loop’s select_biased! to race the reconnect future against an rt.sleep-based timeout (or manage it as a fused future), then preserve the existing replacement/disconnect flow on success and exit through the existing drive termination path on timeout or failure.wacore/src/voip/engine.rs (2)
1594-1597: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t silently drop 1:1 packets when stripping group forwarding envelopes.
on_packetis the direct-call path as well, so runningunwrap_group_forwarding_packetbeforeclassify_relay_packetrisks parsing or reslicing payloads that are not group-forwarding, then dropping them onErr. Keep the group unwrap behind a group-call guard unless this is provably a no-op for all plain 1:1 packet framing.🤖 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 `@wacore/src/voip/engine.rs` around lines 1594 - 1597, Update the packet handling in on_packet so unwrap_group_forwarding_packet is only called for group calls, preserving the original packet for direct 1:1 calls. Ensure classify_relay_packet still receives plain 1:1 packets unchanged and retain the existing early return for malformed group-forwarding envelopes.
1554-1575: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBuild one SRTCP per stream per tick, not one per participant.
A 32-person call is already at the cap, and this code emits a separate sender report for every audio and video reception stat each RTCP tick. Use the SR/RR packet’s reception report count field to batch all participant report blocks into one SR packet per stream, then send the next report packet only if we exceed the per-packet block limit.
🤖 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 `@wacore/src/voip/engine.rs` around lines 1554 - 1575, Update the group_reports handling in the engine tick flow to batch all audio reception reports into SRTCP sender-report packets using the SR/RR reception-report count limit, emitting one packet per stream when possible and additional packets only when the block limit is exceeded. Apply the same batching to active, ungated video reports, while preserving empty-report behavior and existing transmit ordering.
♻️ Duplicate comments (1)
wacore/src/voip/driver.rs (1)
828-875: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAn epoch that arrives before the first roster still hangs up the call.
apply_group_raw_epochreturnsErr(GroupMediaError::Pipeline)for exactly one reachable reason:self.groupisNone(engine.rs line 1033). That is the ordinary startup race — the server's<enc_rekey>beating the first<relay>/roster stanza — and line 869 turns it intobreak 'drive. The pending-epoch buffer in the registry exists to absorb this; the driver shouldn't be euthanizing the call before it gets the chance.Same inversion on the Update arm: everything except the four listed
GroupMediavariants breaks, andGroupMedia(Pipeline)is reachable there too (a control-only engine, or aself_lid/recv_peer_lidthat won't parse as aJid, engine.rs lines 960-968). Flip both to "reject the control, keep the call."🐛 Proposed fix
- Err(error) => { - if matches!(error, crate::voip::GroupMediaError::Pipeline) { - break 'drive; - } - let _ = channels.events.try_send(CallEvent::GroupControlRejected { - control: engine::GroupControlKind::Epoch, - }); - } + // An epoch racing ahead of the first roster is a startup ordering + // artifact, not a broken call: drop the control, never the session. + Err(_) => { + let _ = channels.events.try_send(CallEvent::GroupControlRejected { + control: engine::GroupControlKind::Epoch, + }); + }The existing
rejected_group_control_does_not_terminate_the_drivertest only covers the Reaction arm — please extend it to a pre-rosterRawEpochand aPipeline-producingUpdate.🤖 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 `@wacore/src/voip/driver.rs` around lines 828 - 875, Update the GroupControl handling in the driver so GroupMediaError::Pipeline from both the RawEpoch and Update arms sends GroupControlRejected without breaking the drive loop; preserve termination only for unrelated fatal errors. Extend rejected_group_control_does_not_terminate_the_driver to cover a pre-roster RawEpoch and an Update that produces Pipeline, asserting the driver remains active.
🤖 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/voip/driver.rs`:
- Around line 310-360: Collapse purge_queued_video and
purge_queued_encrypted_media into a shared purge_queued helper that accepts the
queue state and a batch-discard predicate. Keep the existing record_drop
filtering and pending_video accounting, keyframe re-arm, and DroppedMedia return
behavior in the helper, then update both call sites to pass their respective
predicates: video batches for purge_queued_video behavior and non-Control
batches for encrypted-media behavior.
---
Outside diff comments:
In `@src/voip/facade.rs`:
- Around line 916-947: Move the GroupOfferTeardown guard construction in the
call-link join flow to immediately after join_call_link_with_audio succeeds and
before the admission-wait loop. Keep it armed while awaiting listener updates so
cancellation, setup errors, or admission failure send server-side termination;
retain the existing local registry cleanup separately.
In `@wacore/src/voip/driver.rs`:
- Around line 630-637: In the `Output::ReconnectRelay` handling, change the
`awaiting_video_keyframe` assignment to require a keyframe after clearing queued
video during relay migration. Also update `commit_group_allocate` to call
`require_video_keyframe()` when the endpoint changes, keeping the outbound gate
closed until a fresh keyframe is available.
- Around line 662-672: The reconnect await in the drive loop must be
time-bounded so a hung dial cannot pause keepalive, playout, inbound draining,
or terminal handling. Update the reconnect handling around transport.reconnect
and the drive loop’s select_biased! to race the reconnect future against an
rt.sleep-based timeout (or manage it as a fused future), then preserve the
existing replacement/disconnect flow on success and exit through the existing
drive termination path on timeout or failure.
In `@wacore/src/voip/engine.rs`:
- Around line 1594-1597: Update the packet handling in on_packet so
unwrap_group_forwarding_packet is only called for group calls, preserving the
original packet for direct 1:1 calls. Ensure classify_relay_packet still
receives plain 1:1 packets unchanged and retain the existing early return for
malformed group-forwarding envelopes.
- Around line 1554-1575: Update the group_reports handling in the engine tick
flow to batch all audio reception reports into SRTCP sender-report packets using
the SR/RR reception-report count limit, emitting one packet per stream when
possible and additional packets only when the block limit is exceeded. Apply the
same batching to active, ungated video reports, while preserving empty-report
behavior and existing transmit ordering.
In `@wacore/src/voip/group_media.rs`:
- Around line 314-329: Update the pending_epochs buffering logic in the
roster_transaction branch to enforce capacity before inserting the new
transaction_id. Evict the existing farthest key when the buffer is full, and if
the incoming transaction_id would be that farthest entry, skip buffering it and
return the existing buffered outcome. Preserve EpochKey validation and normal
insertion behavior when capacity remains.
---
Duplicate comments:
In `@wacore/src/voip/driver.rs`:
- Around line 828-875: Update the GroupControl handling in the driver so
GroupMediaError::Pipeline from both the RawEpoch and Update arms sends
GroupControlRejected without breaking the drive loop; preserve termination only
for unrelated fatal errors. Extend
rejected_group_control_does_not_terminate_the_driver to cover a pre-roster
RawEpoch and an Update that produces Pipeline, asserting the driver remains
active.
🪄 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: d696a67c-2b16-4a2a-a967-4562b616e4c7
📒 Files selected for processing (14)
Cargo.tomlsrc/client/voip.rssrc/handlers/call.rssrc/voip/facade.rswacore/src/stanza/call.rswacore/src/stanza/group_call.rswacore/src/voip/app_data.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/group_audio.rswacore/src/voip/group_media.rswacore/src/voip/registry.rswacore/src/voip/ssrc.rswacore/src/voip/stun.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4694863d2
ℹ️ 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: e19a7a0906
ℹ️ 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: 4aaa0ff08d
ℹ️ 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".
4aaa0ff to
8754d70
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8754d704d9
ℹ️ 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: f16f580cfc
ℹ️ 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: 4054fbee69
ℹ️ 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: 44827d90fe
ℹ️ 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: 214a7089f6
ℹ️ 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
♻️ Duplicate comments (1)
src/voip/facade.rs (1)
753-756: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOur own device is still advertising audio-only capability on a video group call — this is the same bug we already called out.
self_devicehardcodesoffer_capability(false, audio.config().format)at Line 754-755, but the offer three lines down is built withvideo: video.is_some()(Line 784).build_group_usersingroup_call.rshappens to patch the outbound wire bytes of the very first offer for the creator device, but theparticipantswe hand to the registry — and therefore every laterinvite_participant()/ring_participant()call — still carries the wrong audio-only blob for us. Every new person we invite into a video group call after creation gets told we can't do video.place_calland the accept path both get this right by threading through the real video flag; this one still doesn't. Fix it before someone ships a video group call where nobody can see the host.🔧 Advertise the capability we're actually offering
let self_device = GroupCallDevice::new(own_lid.clone()) - .with_capability(1, offer_capability(false, audio.config().format)); + .with_capability(1, offer_capability(video.is_some(), audio.config().format));🤖 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/voip/facade.rs` around lines 753 - 756, Update self_device construction in the group-call setup to pass the actual video-enabled state to offer_capability, matching the video flag used when building the offer. Preserve the existing audio format and ensure the capability stored in participants reflects whether video is_some() for later invite_participant() and ring_participant() calls.
🤖 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/voip/group_media.rs`:
- Around line 287-301: Update the PID-change condition in the group receiver
handling around existing.pid so receiver rebuilding occurs only when both the
existing and device PIDs are present and differ; preserve the seeded direct peer
when transitioning from None to Some. Add coverage to
device_pid_change_rebuilds_its_receive_session for the seeded None-to-Some case,
ensuring it retains the keyed direct-call receiver before an epoch is installed.
---
Duplicate comments:
In `@src/voip/facade.rs`:
- Around line 753-756: Update self_device construction in the group-call setup
to pass the actual video-enabled state to offer_capability, matching the video
flag used when building the offer. Preserve the existing audio format and ensure
the capability stored in participants reflects whether video is_some() for later
invite_participant() and ring_participant() calls.
🪄 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: d510fd36-1946-47ca-9ba2-e376b34b911d
📒 Files selected for processing (6)
src/client/voip.rssrc/voip/facade.rswacore/src/stanza/group_call.rswacore/src/voip/engine.rswacore/src/voip/group_media.rswacore/src/voip/session.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c870fef86
ℹ️ 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: 5
🤖 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/voip/facade.rs`:
- Around line 1040-1043: Add a test covering cancellation while the call-link is
still in CallPhase::WaitingRoom: drive CallLinkCall::start() to that phase,
cancel the registration, and assert that no call-scoped <terminate> stanza is
sent. Keep the existing admitted-path teardown coverage unchanged.
In `@wacore/src/stanza/group_call.rs`:
- Around line 28-29: The waiting-room capacity currently reuses
GROUP_CALL_MAX_PARTICIPANTS, causing parse_waiting_room to reject valid queues
larger than the connected-roster limit. Update MAX_WAITING_ROOM_USERS to use a
separate cap matching the WhatsApp Web waiting-room limit of 128, while
preserving the existing retained-bytes limit and parsing behavior.
In `@wacore/src/voip/driver.rs`:
- Line 443: Update the reconnect timeout path around RELAY_RECONNECT_TIMEOUT and
the 'drive loop to emit a RelayReconnectTimedOut CallEvent before breaking,
adding the event to CallEvent only if it does not already exist. Bound
retired.disconnect().await in the cleanup at the reconnect handling flow with an
appropriate timeout so a wedged transport cannot block loop termination.
In `@wacore/src/voip/engine.rs`:
- Around line 2494-2496: Update the configure_group_at call path to carry the
already-parsed local device JID rather than parsing participant_id back into a
Jid. Remove the local parse-and-return-false fallback around local_device, and
pass the original JID through the relevant caller and callee while preserving
existing behavior for invalid input.
In `@wacore/src/voip/group_media.rs`:
- Line 302: The PID-migration rule is duplicated between receiver rebuilding and
mixer queue cleanup. In wacore/src/voip/group_media.rs lines 302-302, add a
small private helper such as pid_migrated and use it for the existing.pid check;
in lines 236-249, replace the inline receiver.pid filter with the same helper,
ensuring both consumers use identical migration semantics.
🪄 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: 1806e3ac-8722-4f49-9061-21c7b8d98f6c
📒 Files selected for processing (14)
src/client/voip.rssrc/handlers/call.rssrc/voip/facade.rswacore/src/stanza/call.rswacore/src/stanza/group_call.rswacore/src/types/group_call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/group.rswacore/src/voip/group_audio.rswacore/src/voip/group_media.rswacore/src/voip/registry.rswacore/src/voip/stun.rswacore/tests/group_call_builders_test.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/voip/facade.rs (1)
1053-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe admitted-snapshot validation pair is now copy-pasted twice in the same loop.
The bounded wait is the right call here — we were admitted, the relay just hasn't landed, and unbounded parking on
listener.awaitwould leave the user staring at a joining spinner forever. No complaints on the logic.But
call_creatorequality plusensure_call_link_admitted_mediaappears verbatim at Lines 1053-1058 and again at Lines 1075-1080. Two copies of an identity check is exactly how one of them drifts later. Pull it into one small validator and call it on both snapshots.♻️ Extract the admitted-snapshot validator
fn ensure_admitted_snapshot( update: &GroupCallUpdate, expected_creator: &Jid, media: CallLinkMedia, ) -> Result<(), CallError> { if update.call_creator != *expected_creator { return Err(CallError::Response( "call-link admitted snapshot changed call identity".to_string(), )); } ensure_call_link_admitted_media(update, media) }- if update.call_creator != join.call_creator { - return Err(CallError::Response( - "call-link admitted snapshot changed call identity".to_string(), - )); - } - ensure_call_link_admitted_media(&update, self.media)?; + ensure_admitted_snapshot(&update, &join.call_creator, self.media)?; break update;🤖 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/voip/facade.rs` around lines 1053 - 1082, Extract the duplicated admitted-snapshot validation into a small ensure_admitted_snapshot helper using the existing GroupCallUpdate, Jid, CallLinkMedia, and CallError types. Move the call_creator comparison and ensure_call_link_admitted_media invocation into that helper, then call it for both update snapshots before handling relay state or continuing the wait.wacore/src/voip/registry.rs (1)
1759-1792: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winYou take the buffered epoch out of the entry and then throw it away if the enqueue fails.
Line 1759 does
entry.pending_group_epoch.take()before anything is queued. Every failure path after that — a closed receiver ontry_send, or the split replay whereUpdatelands butRawEpochdoesn't — hitsif !queued { return false; }with the epoch already moved out and dropped (zeroized).group_ctl_txis never stored, so the entry is now back in "no media attached" state but permanently without its epoch:pending_group_epoch_transaction_if_currentreportsNone, and the next successfulset_group_control_senderattaches a sender that replays the roster with no key. Group media for that generation never decrypts.The whole point of buffering the epoch is that it survives until the driver actually has it. Put it back when the handoff fails.
🔒 Restore the buffered epoch on every failure path
- let pending_epoch = entry.pending_group_epoch.take(); - let queued = match (update, pending_epoch) { + let pending_epoch = entry.pending_group_epoch.take(); + // The epoch is only consumed once the driver's mailbox has accepted it; any failure must + // leave it retained so a later attachment still installs the latest key. + let mut unqueued_epoch = None; + let queued = match (update, pending_epoch) { (Some(update), Some(epoch)) => { let transition = GroupControl::Transition { update: Box::new(update), epoch, }; if tx.accepts(&transition) { - tx.try_send(transition) + match transition { + GroupControl::Transition { update, epoch } => { + let sent = tx.try_send(GroupControl::Transition { + update, + epoch: epoch.clone(), + }); + if !sent { + unqueued_epoch = Some(epoch); + } + sent + } + _ => false, + } } else { // ... split replay, tracking `unqueued_epoch` the same way } } - (None, Some(epoch)) => tx.try_send(GroupControl::RawEpoch(epoch)), + (None, Some(epoch)) => { + let sent = tx.try_send(GroupControl::RawEpoch(epoch.clone())); + if !sent { + unqueued_epoch = Some(epoch); + } + sent + } .. }; if !queued { + entry.pending_group_epoch = unqueued_epoch; return false; }(
GroupRawEpochisn'tClonetoday — either derive it behind the zeroizingDrop, or restructure to keep the epoch by reference until the send is known to have succeeded.)🤖 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 `@wacore/src/voip/registry.rs` around lines 1759 - 1792, Restore the taken pending epoch whenever the sender handoff in set_group_control_sender fails. Update the logic around pending_epoch, the transition fallback, and the !queued return so both closed-channel failures and partial split-replay failures preserve the epoch in entry.pending_group_epoch; account for GroupRawEpoch’s non-Clone ownership safely, without storing the sender until the complete replay succeeds.
🤖 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/voip/facade.rs`:
- Around line 7021-7032: Replace the fixed ten-yield synchronization in the
timeout test around client.call_registry().phase_if_current and start.await with
the bounded wait pattern already used elsewhere in src/voip/facade.rs. Ensure
the test waits for the relay node or timeout setup with an explicit deadline, so
a missed timer arm fails fast instead of allowing start.await to hang; preserve
the expected CallError::ResponseTimeout assertion.
In `@wacore/src/voip/registry.rs`:
- Around line 653-700: The ringing-budget arithmetic is duplicated with
inconsistent aggregate filters. Add one private CallRegistry helper for
evaluating an entry’s prospective retained bytes against
MAX_RINGING_GROUP_CALL_BYTES, including the shared preview/current-state
calculation and the canonical ringing-roster membership filter, then replace the
inline logic in the shown update path and insert_ringing_group_if_inactive with
calls to that helper.
---
Outside diff comments:
In `@src/voip/facade.rs`:
- Around line 1053-1082: Extract the duplicated admitted-snapshot validation
into a small ensure_admitted_snapshot helper using the existing GroupCallUpdate,
Jid, CallLinkMedia, and CallError types. Move the call_creator comparison and
ensure_call_link_admitted_media invocation into that helper, then call it for
both update snapshots before handling relay state or continuing the wait.
In `@wacore/src/voip/registry.rs`:
- Around line 1759-1792: Restore the taken pending epoch whenever the sender
handoff in set_group_control_sender fails. Update the logic around
pending_epoch, the transition fallback, and the !queued return so both
closed-channel failures and partial split-replay failures preserve the epoch in
entry.pending_group_epoch; account for GroupRawEpoch’s non-Clone ownership
safely, without storing the sender until the complete replay succeeds.
🪄 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: 79f5b225-74f2-4e11-aa9b-9f316dfb2e2a
📒 Files selected for processing (6)
src/voip/facade.rswacore/src/stanza/group_call.rswacore/src/voip/driver.rswacore/src/voip/engine.rswacore/src/voip/group_media.rswacore/src/voip/registry.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d64844095
ℹ️ 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: bb066e27cb
ℹ️ 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: 8209b208e2
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/src/voip/group_media.rs (1)
795-806: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
is_connected()here — this is the last site still spelling the literal by hand.
GroupCallParticipant::is_connected()exists for exactly this, andremote_group_pidsandgroup_roster_contains_participantinwacore/src/voip/engine.rsboth already call it. This filter decides who gets a media receiver; the engine's filter decides who we subscribe to on the relay. Those two sets must be identical, and right now one is a method call and the other is a string comparison. The day "connected" gains a sibling state, somebody fixes one of them.♻️ Proposed fix
update .participants .iter() - .filter(|participant| participant.state.as_deref() == Some("connected")) + .filter(|participant| participant.is_connected()) .flat_map(|participant| {🤖 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 `@wacore/src/voip/group_media.rs` around lines 795 - 806, In the participant filter within the media-receiver update chain, replace the manual state comparison with GroupCallParticipant::is_connected(). Keep the existing device PID and local-device filters unchanged so receiver selection matches the engine’s connected-participant logic.src/client/voip.rs (1)
1188-1188: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRelease the pre-ACK join lane right after registration binds the call id.
register_call_link_session()publishes the generation and drains its staged admission state, but_pending_join_lanestays held throughwaiting_room_heartbeat, the second admission sync, and heartbeat scheduling. Everybuffer_pending_call_link_*path short-circuits once a call id has a registry generation, so holding this lane any further only makes another join wait on post-registration transport/heartbeat work. Drop_pending_join_laneimmediately after the registration call succeeds; keep_pending_joinalive because the staging-buffer guard is still needed until registration completes.🤖 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/client/voip.rs` at line 1188, Release the `_pending_join_lane` guard immediately after `register_call_link_session()` succeeds, before `waiting_room_heartbeat`, the second admission sync, or heartbeat scheduling. Keep `_pending_join` alive until registration is fully complete so staging remains protected.
🤖 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/types/group_call.rs`:
- Around line 434-440: Replace the ineffective needs_drop::<GroupCallRelay>()
assertion in group_call_relay_always_scrubs_on_drop with a test of the actual
scrubbing behavior. Expose a pub(crate) scrub(&mut self) method that performs
the zeroization and have Drop delegate to it, then assert the relevant relay
secret buffers are cleared through scrub while preserving Drop’s invocation of
that method.
---
Outside diff comments:
In `@src/client/voip.rs`:
- Line 1188: Release the `_pending_join_lane` guard immediately after
`register_call_link_session()` succeeds, before `waiting_room_heartbeat`, the
second admission sync, or heartbeat scheduling. Keep `_pending_join` alive until
registration is fully complete so staging remains protected.
In `@wacore/src/voip/group_media.rs`:
- Around line 795-806: In the participant filter within the media-receiver
update chain, replace the manual state comparison with
GroupCallParticipant::is_connected(). Keep the existing device PID and
local-device filters unchanged so receiver selection matches the engine’s
connected-participant logic.
🪄 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: 07ff4f73-f4f5-45d9-92dc-03e0ebfb341c
📒 Files selected for processing (11)
src/client/voip.rssrc/handlers/call.rssrc/voip/facade.rswacore/Cargo.tomlwacore/src/stanza/call.rswacore/src/types/call.rswacore/src/types/group_call.rswacore/src/voip/engine.rswacore/src/voip/group_media.rswacore/src/voip/registry.rswacore/tests/incoming_call_builder_test.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/voip/engine.rs (1)
971-1050: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard live direct-to-group promotion before changing SSRCs.
configure_group()rejects a started engine, butapply_group_update()can still callconfigure_group_at()from the promotion path even afterstart()returns and RTP may already be sending. Add a started/allocated/reconnecting guard before reassigningmedia.pipe.set_send_ssrc(...)andself_participant_id, or ensure the promoted participant identity is provably identical to the outgoing 1:1 identity before swapping stream SSRCs.🤖 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 `@wacore/src/voip/engine.rs` around lines 971 - 1050, The direct-to-group promotion path around configure_group must not reassign participant identity or stream SSRCs once the engine is started, allocated, or reconnecting. Add the existing lifecycle-state guard before updating self_participant_id, media.self_lid, or calling media.pipe.set_send_ssrc (including video), and return the appropriate existing error; alternatively, only permit the swap when the promoted identity is proven identical to the outgoing 1:1 identity.
🤖 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/client/voip.rs`:
- Around line 3508-3591: Strengthen
registered_call_link_releases_the_unknown_id_lane_before_heartbeat by using the
existing GatedTransport pattern to block the first heartbeat send after its ACK.
Observe that the heartbeat has started, launch the second join, and assert its
link_join request arrives while the transport remains gated; then release the
gate and clean up both tasks, preserving the test’s intended ordering proof.
---
Outside diff comments:
In `@wacore/src/voip/engine.rs`:
- Around line 971-1050: The direct-to-group promotion path around
configure_group must not reassign participant identity or stream SSRCs once the
engine is started, allocated, or reconnecting. Add the existing lifecycle-state
guard before updating self_participant_id, media.self_lid, or calling
media.pipe.set_send_ssrc (including video), and return the appropriate existing
error; alternatively, only permit the swap when the promoted identity is proven
identical to the outgoing 1:1 identity.
🪄 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: afd985f8-ab9a-473b-ab1f-588ba274d6d9
📒 Files selected for processing (6)
src/client/voip.rssrc/voip/facade.rswacore/src/types/group_call.rswacore/src/voip/engine.rswacore/src/voip/group_media.rswacore/src/voip/session.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f770703b06
ℹ️ 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
whatsapp-rust/wacore/src/voip/h264.rs
Lines 242 to 243 in e274d4b
Drop the entire oversized H.264 access unit
When an authenticated participant sends an access unit whose accumulated NAL data crosses H264_MAX_AU_BYTES—for example, SPS/PPS followed by an oversized IDR—this silently ignores only the offending NAL. The marker path then publishes the previously buffered prefix as a complete frame, potentially reporting a parameter-set-only frame as a keyframe and leaving the decoder stalled or corrupted; mark the whole access unit invalid and discard it at the boundary instead of emitting its prefix.
ℹ️ 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: 27a40777f0
ℹ️ 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: 5dea6bd4fa
ℹ️ 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".
1 participant
Summary
Impact
Embedders can create and receive group calls, promote an active direct call into an ad-hoc group call, join by group JID or call link, manage waiting-room participants, and consume participant-aware media through the existing VoIP surface. Applications use typed APIs rather than constructing protocol nodes directly.
The lifecycle is generation-scoped and cancellation-safe: registration, admission, rekey publication, relay refresh, participant controls, and teardown are serialized against replacement races. Authoritative snapshots are validated before publication, call-link admission requires the exact connected local device (including an accepted PN alias), and bounded queues preserve critical roster, epoch, and relay transitions under backpressure. Buffered or asynchronously decrypted epochs remain retained until the exact generation and media-driver handoff succeeds, including ACK/Signal-decrypt races, closed mailboxes, and partial split-replay failures.
Validation
cargo fmt --all -- --checkcargo test -p wacore --features voip-mlow --lib— 1,767 passed; 2 ignoredcargo test -p wacore --features voip --lib— 1,594 passed; 1 ignoredcargo test -p whatsapp-rust --features voip-mlow --lib— 1,445 passed; 1 ignoredcargo test -p wacore --features voip-mlow --test group_call_builders_test— 1 passedcargo test -p wacore --test incoming_call_builder_test— 1 passedcargo clippy --workspace --all-targets -- -D warningscargo clippy --workspace --all-features --all-targets -- -D warningscargo clippy -p wacore --features voip-mlow --all-targets -- -D warningscargo clippy -p whatsapp-rust --features voip-mlow --all-targets -- -D warningscargo clippy -p whatsapp-rust --no-default-features --lib -- -D warningscargo check -p wacore --no-default-featuresAutomated coverage includes generated typed stanza dispatch and builder-backed construction for frozen top-level and nested event payloads, public builder compatibility (including IncomingCall), admitted-roster sender identity and PN-alias media authentication, exact-device epoch fanout, provisional join-buffer cleanup before unknown-call-id lane release, reuse of an already decrypted epoch when registration overtakes pending buffering, admitted call-link cancellation, routed sibling dismissal, local PN-alias media exclusion, unconditional relay-secret scrubbing, registration, generation-bound group-invite preaccept and rejection, and pre-fanout ACK races, attachable early group-invite acceptance, exact-device and PN-alias call-link admission, pre-media snapshot byte bounds, waiting-room lifecycle and cancellation, bounded post-admission relay setup, bounded-buffer saturation and ringing-snapshot growth, participant authorization and PN/LID-alias invite membership, live direct-to-group PN-alias promotion without sender rotation, codec/capability retention, relay allocation, refresh, reconnect timeouts and cleanup, atomic rekeying, failed and partial epoch-handoff retry retention, partial-fanout Signal consistency, local-LID/PN epoch-fanout exclusion, old-key media gating, roster device ownership and route validation, PID migration, reaction-session deduplication, participant-scoped video controls, RTCP/data routing, and cancellation-safe teardown.
Live service validation still requires a compatible account and a multi-device call session.
Binary size
The expected feature-size cost is tracked by the repository Binary Size workflow and explicitly accepted for this PR with the
size-increase-oklabel.