fix(voip): prevent stale audio-to-video upgrades - #1051
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughVoIP video upgrades now use generation-scoped transition state and tokens. Typed video events expose tokens, acceptance validates request freshness, timeout and teardown paths release endpoints, and the CLI supports token-based manual or automatic peer acceptance. ChangesVoIP video upgrade lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant CallHandler
participant CallRegistry
participant CallHandle
participant VideoEndpoints
Peer->>CallHandler: typed video upgrade request
CallHandler->>CallRegistry: apply peer video state
CallRegistry-->>CallHandler: current VideoUpgradeToken
CallHandler-->>CallHandle: VideoStateChanged with token
CallHandle->>CallRegistry: validate token
CallHandle->>VideoEndpoints: attach source and sink
CallHandle->>CallRegistry: complete peer request
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c48402585
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/call.rs (1)
223-387: 🩺 Stability & Availability | 🔵 TrivialI went through this whole transition block against every test you added (
cancelled_peer_request_invalidates_its_acceptance_token,video_transition_stays_serialized_through_enabled_announcement,refused_upgrade_tears_down_when_typed_ack_send_fails,committed_video_state_supersedes_diagnostics_when_event_queue_is_full, etc.) and it holds together. Teardown correctly still fires for cancel/reject/disable/error even when the typed ack send itself fails, and the event/dispatch gating correctly stays scoped toVideoStateactions only. Solid work — this is what "ship it right the first time" looks like.One thing worth being aware of operationally:
_transition_guardis held across theclient.send_node(...)awaits for the accept/enabled handshake stanzas, which means a peer's OWN incoming<video state>signal (or a local user action on the same call) is serialized behind those two outbound sends completing. That's clearly intentional — it mirrors WA's native stream mutex and is explicitly exercised byvideo_transition_stays_serialized_through_enabled_announcement— but on a slow/lossy connection this means video signaling on this call is briefly head-of-line-blocked behind the outbound stanza round-trip. Worth keeping in mind if upgrade latency ever gets flagged in the field.🤖 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 223 - 387, The transition serialization in the video-state handling block intentionally holds _transition_guard across the awaited accept and enabled client.send_node calls. Preserve this locking scope and ordering so peer signals and local actions remain serialized through the handshake announcements; make no code change unless future requirements explicitly address the resulting latency.
🤖 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 `@examples/voip-cli/src/main.rs`:
- Around line 1424-1431: Update the auto_video branch in the call event listener
to spawn accept_peer_video as a background task instead of awaiting camera
initialization inline, allowing the signaling loop to continue processing
events. Move the success/failure logging and activate_peer_video_request call
into the spawned task, preserving the existing behavior after acceptance
completes.
- Around line 1276-1285: Update activate_peer_video_request to use
video_ui.get_mut(call_id) and mutate the existing entry in place when it matches
VideoUi::PendingPeerRequest(request), replacing it with VideoUi::Active without
calling insert or allocating a new String key.
---
Outside diff comments:
In `@src/handlers/call.rs`:
- Around line 223-387: The transition serialization in the video-state handling
block intentionally holds _transition_guard across the awaited accept and
enabled client.send_node calls. Preserve this locking scope and ordering so peer
signals and local actions remain serialized through the handshake announcements;
make no code change unless future requirements explicitly address the resulting
latency.
🪄 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: babdfedb-12c8-45cb-8728-d5cb23989fbf
📒 Files selected for processing (10)
Cargo.tomlexamples/voip-cli/src/main.rssrc/client/voip.rssrc/handlers/call.rssrc/voip/facade.rssrc/voip/mod.rswacore/src/types/call.rswacore/src/voip/engine.rswacore/src/voip/mod.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: ad0d0def83
ℹ️ 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".
| tokio::spawn(async move { | ||
| if let Err(e) = accept_peer_video(&handle, request).await { |
There was a problem hiding this comment.
In --video auto-accept mode, a peer cancel/re-request while the first accept_peer_video is still waiting for ffmpeg's first IDR now starts another detached task; the stale task is not cancelled and the token is not checked until after spawn_video_source opens /dev/video0, so the new request can fail with the camera still busy. Keep the task tied to the current token/cancel event or claim the request before opening the source.
Useful? React with 👍 / 👎.
| self.release_local_video(); | ||
| self.client_registry | ||
| .stop_local_video(&self.call_id, self.generation); |
There was a problem hiding this comment.
When a bidirectional video call is reduced only on our side, stop_local_video leaves the peer direction active, but release_local_video() detaches endpoints and sends VideoControl::Disable; the driver maps that to CallEngine::disable_video(), which ignores inbound video. As a result, stop_video() also stops rendering the peer's still-active stream until another upgrade re-enables the whole plane.
Useful? React with 👍 / 👎.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 360b71a143
ℹ️ 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".
| pub struct VideoUpgradeToken { | ||
| generation: u64, | ||
| epoch: u64, | ||
| } |
There was a problem hiding this comment.
When an application operates multiple Client instances, each registry starts its generation counter at zero and each call starts its request epoch at zero, so the first peer request in two registries produces the identical { generation: 0, epoch: 1 } token. accept_video validates only these two values against the target handle's registry, allowing a token received from one client to accept and attach endpoints to a different client's pending request; include registry/request identity that cannot collide across client instances.
Useful? React with 👍 / 👎.
1 participant
Summary
accept_videoto an exact peer request token so cancelled or superseded upgrades cannot attach camera endpointsRoot cause
Camera preparation waits for the first decodable IDR outside the signaling path. In the reported trace that preparation crossed the peer timeout, so the peer cancellation overtook the user action. The old code then completed the stale accept and retained
/dev/video0, making the next upgrade fail withDevice or resource busy.The native APK/WASM behavior guards acceptance on the peer still being in Request state, serializes state transitions through a stream mutex, and expires local requests after five seconds. This change mirrors those invariants with a per-generation transition lock plus monotonic request epochs.
Verification
cargo fmt --allcargo clippy --all --tests --features voip -- -D warningscargo test --workspace --exclude e2e-testscargo test --features voip voip::facade::tests -- --nocapturecargo test --features voip handlers::call::tests -- --nocapturecargo test -p wacore --features voip voip::registry -- --nocapturecargo test -p whatsapp-rust-voip-clicargo test --allwas also attempted; only the external E2E crate failed because its documented mock server was not running (Connection refused).