Skip to content

Commit 0dccc82

Browse files
intendednullclaude
andauthored
fix(voice): make calls actually deliver media — five root-cause fixes (#673)
Voice/video calls connected (roster visible) but no audio/video ever reached the remote peer. Root-cause investigation (docs/reports/2026-06-07-voice-media-connectivity-investigation.md) found five independent, compounding bugs; all but cross-NAT traversal are fixed here and proven by a new 2-peer Playwright media test. RC0 — voice wire addressed by channel name, gates validate UUIDs. VoiceJoin/VoiceLeave/VoiceSignal carried the UI's channel *name*, but the SEC-V-03 existence gates check ServerState.channels (keyed by UUID), so every voice message was dropped — presence and signaling never crossed at all. The wire now carries the canonical channel_id: senders resolve name->id (channel_id_for_voice), the listener gates resolve id->name and keep voice state / ClientEvents name-keyed to match the UI. Security tests updated to the UUID-on-wire contract. RC2 — 4 KB SIGNALING_CAP silently dropped video SDP. Video offers run 5-15 KB; unpack_wire discarded them post-decode so the offerer never got an answer. VoiceSignal now has a dedicated 64 KB SDP_CAP (still inside the 256 KB transport cap); oversize is still rejected. RC3 — early remote ICE candidates were lost. addIceCandidate rejects while remoteDescription is null (browsers don't buffer remote candidates) and the rejection was swallowed with `let _`. Candidates arriving before the offer/answer (gossip has no ordering; ICE handling was synchronous while offer/answer were spawned) are now queued in a per-peer PendingIceCandidates buffer and flushed after setRemoteDescription resolves; rejections are logged. RC4 — RefCell<VoiceManager> borrow held across await. The "safe on single-threaded WASM, no preemption" comment was wrong: await is a yield point, and a concurrent VoiceSignal task could double-borrow and panic mid-negotiation. VoiceManager now owns its state behind interior mutability with &self methods; the handle is Rc<VoiceManager> (no outer RefCell), and the clippy::await_holding_refcell_ref allows are gone, so the lint enforces the invariant from now on. Perfect negotiation — the collision check tested only making_offer, not signalingState != stable (the canonical MDN condition), which breaks renegotiation (screen share added mid-call). Extracted as the pure should_ignore_offer(polite, making_offer, stable) and fixed. Also: self-offer filter (don't offer to our own echoed VoiceJoin), ICE/connection state-change logging (failures were invisible), and TURN credential plumbing — build_rtc_config only ever set urls, so it could not drive TURN even if configured; a new window.__WILLOW_ICE_SERVERS knob carries {urls, username, credential} (legacy __WILLOW_STUN_URLS still honored). NOT fixed here (follow-up, needs infra + spec): cross-NAT traversal. iceServers stays empty by default (privacy-first, issue #179); the plan is self-hosted coturn beside the relay — see the report's recommendation. Same-host/LAN calls work without it; the "iroh relay path for ICE" idea was researched and is infeasible for browsers (relay-only WebSocket, not TURN/ICE-compatible). Tests: ~10 KB SDP wire round-trip (native); ICE buffer, collision rule, TURN credential propagation, ICE-server config knobs (wasm-pack); new e2e/voice-video.spec.ts under a voice-chrome Playwright project with fake media devices proving remote audio both ways + screen-share renegotiation over a real RTCPeerConnection. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c69d023 commit 0dccc82

13 files changed

Lines changed: 1274 additions & 282 deletions

File tree

crates/client/src/listeners.rs

Lines changed: 47 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -644,19 +644,24 @@ pub(crate) async fn process_received_message<T: TopicHandle>(
644644
// per channel) dimensions of `VoiceState.participants`.
645645
// Without these gates, any signed peer can flood arbitrary
646646
// `channel_id`s and exhaust receiving clients' memory.
647-
let known = willow_actor::state::select(&ctx.event_state, {
647+
//
648+
// The wire carries the canonical `channel_id` (UUID); voice state
649+
// and UI events are keyed by the channel *name* (the whole UI is
650+
// name-keyed). Resolve id -> name here: `None` both fails the
651+
// existence gate and means "unknown channel".
652+
let name = willow_actor::state::select(&ctx.event_state, {
648653
let ch = channel_id.clone();
649-
move |es| es.channels.contains_key(&ch)
654+
move |es| es.channels.get(&ch).map(|c| c.name.clone())
650655
})
651656
.await;
652-
if !known {
657+
let Some(name) = name else {
653658
tracing::debug!(
654659
%signer, channel_id = %channel_id,
655660
"dropping VoiceJoin: channel_id not in ServerState.channels"
656661
);
657662
return;
658-
}
659-
let ch = channel_id.clone();
663+
};
664+
let ch = name.clone();
660665
let inserted = willow_actor::state::mutate(&ctx.voice, move |v| {
661666
let new_channel = !v.participants.contains_key(&ch);
662667
if new_channel && v.participants.len() >= MAX_VOICE_CHANNELS {
@@ -680,7 +685,7 @@ pub(crate) async fn process_received_message<T: TopicHandle>(
680685
warn_if_err(
681686
ctx.event_broker
682687
.do_send(willow_actor::Publish(ClientEvent::VoiceJoined {
683-
channel_id,
688+
channel_id: name,
684689
peer_id,
685690
})),
686691
"event_broker.do_send Publish(VoiceJoined)",
@@ -695,19 +700,21 @@ pub(crate) async fn process_received_message<T: TopicHandle>(
695700
// UI. The `participants` map is unaffected even without the
696701
// gate (`get_mut` returns None), but the event-broker fanout
697702
// would still leak attacker-controlled `channel_id`s.
698-
let known = willow_actor::state::select(&ctx.event_state, {
703+
// Resolve the wire `channel_id` (UUID) to the name used by voice
704+
// state + UI events; `None` fails the existence gate.
705+
let name = willow_actor::state::select(&ctx.event_state, {
699706
let ch = channel_id.clone();
700-
move |es| es.channels.contains_key(&ch)
707+
move |es| es.channels.get(&ch).map(|c| c.name.clone())
701708
})
702709
.await;
703-
if !known {
710+
let Some(name) = name else {
704711
tracing::debug!(
705712
%signer, channel_id = %channel_id,
706713
"dropping VoiceLeave: channel_id not in ServerState.channels"
707714
);
708715
return;
709-
}
710-
let ch = channel_id.clone();
716+
};
717+
let ch = name.clone();
711718
willow_actor::state::mutate(&ctx.voice, move |v| {
712719
if let Some(p) = v.participants.get_mut(&ch) {
713720
p.remove(&peer_id);
@@ -717,7 +724,7 @@ pub(crate) async fn process_received_message<T: TopicHandle>(
717724
warn_if_err(
718725
ctx.event_broker
719726
.do_send(willow_actor::Publish(ClientEvent::VoiceLeft {
720-
channel_id,
727+
channel_id: name,
721728
peer_id,
722729
})),
723730
"event_broker.do_send Publish(VoiceLeft)",
@@ -734,22 +741,22 @@ pub(crate) async fn process_received_message<T: TopicHandle>(
734741
// event handlers. Signals do not mutate `participants`
735742
// directly, but the existence check shuts the same fanout
736743
// gap as Join/Leave.
737-
let known = willow_actor::state::select(&ctx.event_state, {
744+
let name = willow_actor::state::select(&ctx.event_state, {
738745
let ch = channel_id.clone();
739-
move |es| es.channels.contains_key(&ch)
746+
move |es| es.channels.get(&ch).map(|c| c.name.clone())
740747
})
741748
.await;
742-
if !known {
749+
let Some(name) = name else {
743750
tracing::debug!(
744751
%signer, channel_id = %channel_id,
745752
"dropping VoiceSignal: channel_id not in ServerState.channels"
746753
);
747754
return;
748-
}
755+
};
749756
warn_if_err(
750757
ctx.event_broker
751758
.do_send(willow_actor::Publish(ClientEvent::VoiceSignal {
752-
channel_id,
759+
channel_id: name,
753760
from_peer: signer,
754761
signal,
755762
})),
@@ -1604,6 +1611,18 @@ mod tests {
16041611
.expect("test_client must seed at least one channel")
16051612
}
16061613

1614+
/// The display name of the first seeded channel. Voice state + events are
1615+
/// keyed by name (the wire carries the UUID `channel_id`, which the
1616+
/// listener resolves to this name), so state assertions look up by name.
1617+
async fn known_channel_name<N: willow_network::Network>(client: &ClientHandle<N>) -> String {
1618+
let snap = client.state_snapshot().await;
1619+
snap.channels
1620+
.values()
1621+
.next()
1622+
.map(|c| c.name.clone())
1623+
.expect("test_client must seed at least one channel")
1624+
}
1625+
16071626
/// VoiceJoin against an unknown `channel_id` must not mutate
16081627
/// `VoiceState.participants` (defends against attackers flooding
16091628
/// random channel ids until memory is exhausted).
@@ -1652,7 +1671,10 @@ mod tests {
16521671
.await
16531672
.expect("subscribe must succeed");
16541673
let ctx = make_ctx(&client);
1674+
// Wire is addressed by canonical channel_id (UUID); voice state is
1675+
// keyed by the resolved channel name.
16551676
let channel_id = known_channel_id(&client).await;
1677+
let channel_name = known_channel_name(&client).await;
16561678

16571679
let attacker = willow_identity::Identity::generate();
16581680
let attacker_id = attacker.endpoint_id();
@@ -1665,7 +1687,7 @@ mod tests {
16651687

16661688
let stored = poll_until_some(
16671689
|| {
1668-
let ch = channel_id.clone();
1690+
let ch = channel_name.clone();
16691691
willow_actor::state::select(&client.voice_state_addr, move |v| {
16701692
v.participants
16711693
.get(&ch)
@@ -1679,7 +1701,7 @@ mod tests {
16791701
assert_eq!(
16801702
stored,
16811703
Some(1),
1682-
"VoiceJoin on a known channel must record the participant"
1704+
"VoiceJoin on a known channel must record the participant (keyed by name)"
16831705
);
16841706
}
16851707

@@ -1748,10 +1770,12 @@ mod tests {
17481770
.await
17491771
.expect("subscribe must succeed");
17501772
let ctx = make_ctx(&client);
1773+
// Wire is addressed by channel_id (UUID); state is keyed by name.
17511774
let channel_id = known_channel_id(&client).await;
1775+
let channel_name = known_channel_name(&client).await;
17521776

1753-
// Fill the cap with synthetic participants on the real channel.
1754-
let cap_channel = channel_id.clone();
1777+
// Fill the cap with synthetic participants on the real channel (by name).
1778+
let cap_channel = channel_name.clone();
17551779
willow_actor::state::mutate(&client.voice_state_addr, move |v| {
17561780
let set = v.participants.entry(cap_channel).or_default();
17571781
for _ in 0..MAX_PARTICIPANTS_PER_CHANNEL {
@@ -1763,7 +1787,7 @@ mod tests {
17631787
// Confirm preconditions.
17641788
let pre = voice_snapshot(&client).await;
17651789
assert_eq!(
1766-
pre.participants.get(&channel_id).map(|s| s.len()),
1790+
pre.participants.get(&channel_name).map(|s| s.len()),
17671791
Some(MAX_PARTICIPANTS_PER_CHANNEL),
17681792
"test setup must fill participants to cap"
17691793
);
@@ -1781,7 +1805,7 @@ mod tests {
17811805
let snap = voice_snapshot(&client).await;
17821806
let set = snap
17831807
.participants
1784-
.get(&channel_id)
1808+
.get(&channel_name)
17851809
.expect("channel set must still exist");
17861810
assert_eq!(
17871811
set.len(),

crates/client/src/mutations.rs

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,30 @@ impl<N: willow_network::Network> ClientMutations<N> {
156156
channel_id.ok_or_else(|| anyhow::anyhow!("channel not found: {channel}"))
157157
}
158158

159+
/// Resolve a channel reference (already a `channel_id`, or a display name)
160+
/// to the canonical `channel_id` (UUID) used on the wire.
161+
///
162+
/// Voice signaling (`VoiceJoin`/`VoiceLeave`/`VoiceSignal`) must address
163+
/// channels by their canonical id so the receiver's existence gate — which
164+
/// validates against `ServerState.channels` (keyed by id) — accepts them.
165+
/// The UI keys voice state by name, so this accepts either form: an exact
166+
/// id match wins, otherwise it falls back to a name lookup. Returns `None`
167+
/// if the channel is unknown.
168+
pub(crate) async fn channel_id_for_voice(&self, channel: &str) -> Option<String> {
169+
let ch = channel.to_string();
170+
willow_actor::state::select(&self.event_state, move |es| {
171+
if es.channels.contains_key(&ch) {
172+
Some(ch.clone())
173+
} else {
174+
es.channels
175+
.iter()
176+
.find(|(_, c)| c.name == ch)
177+
.map(|(id, _)| id.clone())
178+
}
179+
})
180+
.await
181+
}
182+
159183
/// Sync event_state mirror from ManagedDag, persist, and emit
160184
/// ClientEvents. Called after build_event succeeds — ManagedDag has
161185
/// already applied the event to its internal state atomically.
@@ -699,21 +723,33 @@ impl<N: willow_network::Network> ClientMutations<N> {
699723

700724
impl<N: willow_network::Network> ClientMutations<N> {
701725
/// Join a voice channel.
702-
pub async fn join_voice(&self, channel_id: &str) {
726+
///
727+
/// `channel` is a channel name (the UI's identifier) or a `channel_id`.
728+
/// Local voice state stays keyed by the channel *name* (consistent with the
729+
/// name-keyed UI); the broadcast `VoiceJoin` carries the canonical
730+
/// `channel_id` (UUID) so remote peers' existence gate accepts it.
731+
pub async fn join_voice(&self, channel: &str) {
703732
let in_voice =
704733
willow_actor::state::select(&self.voice, |v| v.active_channel.is_some()).await;
705734
if in_voice {
706735
self.leave_voice().await;
707736
}
708-
let ch = channel_id.to_string();
737+
// Local voice state is keyed by the caller's reference (the UI's
738+
// channel name), so the UI — which reads the same key — matches.
709739
let my_peer_id = self.identity.endpoint_id();
740+
let ch = channel.to_string();
710741
willow_actor::state::mutate(&self.voice, move |v| {
711742
v.active_channel = Some(ch.clone());
712743
v.participants.entry(ch).or_default().insert(my_peer_id);
713744
})
714745
.await;
746+
// Broadcast addressed by canonical channel_id so peers accept it.
747+
let Some(channel_id) = self.channel_id_for_voice(channel).await else {
748+
tracing::warn!(%channel, "join_voice: unknown channel, not broadcasting");
749+
return;
750+
};
715751
let msg = ops::WireMessage::VoiceJoin {
716-
channel_id: channel_id.to_string(),
752+
channel_id,
717753
peer_id: my_peer_id,
718754
};
719755
if let Some(data) = ops::pack_wire(&msg, &self.identity) {
@@ -736,8 +772,14 @@ impl<N: willow_network::Network> ClientMutations<N> {
736772
})
737773
.await;
738774
if let Some(ch) = maybe_ch {
775+
// `ch` is the local key (channel name); address the wire message
776+
// by canonical channel_id so peers' existence gate accepts it.
777+
let Some(channel_id) = self.channel_id_for_voice(&ch).await else {
778+
tracing::warn!(channel = %ch, "leave_voice: unknown channel, not broadcasting");
779+
return;
780+
};
739781
let msg = ops::WireMessage::VoiceLeave {
740-
channel_id: ch,
782+
channel_id,
741783
peer_id: my_peer_id,
742784
};
743785
if let Some(data) = ops::pack_wire(&msg, &self.identity) {

crates/client/src/voice.rs

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,23 @@ impl<N: willow_network::Network> ClientHandle<N> {
4040
willow_actor::state::select(&self.voice_state_addr, |v| v.deafened).await
4141
}
4242

43-
pub fn send_voice_signal(
43+
/// Send a WebRTC signaling message to a peer.
44+
///
45+
/// `channel` is the UI's channel reference (name) or a `channel_id`; it is
46+
/// resolved to the canonical `channel_id` (UUID) so the receiver's
47+
/// existence gate accepts it. Async because resolution reads event state.
48+
pub async fn send_voice_signal(
4449
&self,
45-
channel_id: &str,
50+
channel: &str,
4651
target: willow_identity::EndpointId,
4752
signal: ops::VoiceSignalPayload,
4853
) {
54+
let Some(channel_id) = self.mutation_handle.channel_id_for_voice(channel).await else {
55+
tracing::warn!(%channel, "send_voice_signal: unknown channel");
56+
return;
57+
};
4958
let msg = ops::WireMessage::VoiceSignal {
50-
channel_id: channel_id.to_string(),
59+
channel_id,
5160
target_peer: target,
5261
signal,
5362
};

0 commit comments

Comments
 (0)